feat(eval): add an Improvement recommendations section to the eval report

[Amitfre15]

Summary

Implements the "Improvement recommendations" section from the parent epic (#2087), per
docs/evaluation/eval-framework.md §9.1 — surfaces findings in the per-cell eval report as
concrete, targeted actions rather than a bare statement of which metrics failed.

• eval/recommendations.py — deterministically gathers evidence for six finding types
  (elevated over-grant, elevated under-grant, sensitivity-family failure, invariance-family
  failure, consistency disagreement, Scale correctness mistake) from data the suites already
  record, each EvidenceCase carrying the concrete scenario(s)/cell(s) that produced it. A
  category with no supporting evidence in a given run is omitted entirely, never printed as a
  vacuous pass statement.
• One batched structured-LLM call (_draft_patterns) clusters same-root-cause cases into
  distinct, case-specific recommendations instead of one templated blurb per failing scenario,
  with a deterministic, clearly-labeled fallback grouping (_fallback_recommendations) when the
  LLM call fails or returns nothing usable — the section is never silently blank or fabricated.
• under_grant_cases also surfaces a pair that is simultaneously granted AND denied (e.g. a
  coarse scope split by the auditor into one approved and one best-effort-rejected
  sub-decision) — previously such a pair landed in incorrectly_denied without ever reaching
  under_grants, so the finding never became an EvidenceCase.
• eval/dashboard/ — the trend-log writer/reader and the dashboard renderer (plus their
  data/output files) moved under one package, a sibling of eval/reports/, instead of loose at
  the eval/ top level; fixes dashboard.py's reports_dir default, which was silently wrong
  once the module no longer sat beside eval/reports/.
• eval/test_recommendations.py — 23 new unit tests covering each evidence-gathering
  function, the LLM-draft/fallback path, and the end-to-end build_recommendations assembly.

Acceptance criteria (from #2472)

• The per-cell report gains an "Improvement recommendations" section populated after every run.
• All six finding-type categories are implemented and demonstrable against real evidence
  (Correctness over/under-grant, Robustness sensitivity/invariance, Consistency disagreement,
  Scale correctness degradation).
• A category with no supporting evidence in a given run is omitted entirely, not printed as a
  vacuous pass statement.
• Every printed recommendation references the concrete scenario(s)/cell(s) behind it.

Test plan

• .venv/bin/pytest eval/test_recommendations.py -v — 23 new unit tests pass
• .venv/bin/pytest — full default offline suite, no regressions
• Verified live against a real correctness-e2e run that hit the simultaneously-granted-and-denied
  pair case, confirming the finding now reaches the recommendations section

Out of scope

• Wiring new finding types beyond the six specified in the issue.
• Changing how any suite scores or gates — this only consumes already-recorded evidence.

Closes rossoctl/rossoctl#2472

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

[oblinder]

Code review: three findings, all in the new improvement-recommendations code. All three are plausible: I traced each one through the code, but I did not reproduce them. The changes are eval-only, so no product code is affected.

The move of the dashboard and trend log into eval/dashboard/ looks correct: imports, default paths, the .gitignore entry and the python -m eval.dashboard.dashboard command all match the new layout.

Suggested before merge: the None guard at recommendations.py:336 (one-line fix) and a check on the new trend_log.jsonl row. The other two findings can go to a follow-up issue.

[oblinder]

result.patterns is read outside the try. If the structured-output call returns None (for example, the model answers in plain text), this raises AttributeError instead of returning []. The fallback grouping is then skipped, and the broad except in pytest_sessionfinish removes the whole recommendations section.

Suggested fix: return [] when result is None, or move the attribute access into the try.

[Amitfre15]

Fixed — _draft_patterns now returns [] when result is None, before touching .patterns (commit c301a57).

[oblinder]

The fallback grouping runs only when no LLM pattern survives validation. If the LLM covers only some of the failing cases, or gives a made-up case id for one of them, the uncovered cases are dropped from the section and nothing says so. This goes against the module docstring, which says the section is "never silently blank".

Suggested fix: after this loop, collect the case ids that no pattern covers and send them through _fallback_recommendations.

[Amitfre15]

Fixed — build_recommendations now tracks exactly which case ids a surviving pattern covers and runs the deterministic fallback over whatever's left uncovered, so an omitted or hallucinated-id case no longer silently drops out of the section (commit c301a57).

[oblinder]

A multi-line LLM body is wrapped in a fence and is not escaped. If the body has its own code block, the inner closing ends the outer fence early. The rest of the report then renders incorrectly, and dashboard.parse_report can attach stray bullets to the last scenario entry.

Suggested fix: use a fence that is longer than the longest backtick run in the body (for example ```` or more), or use a ~~~ fence.

[Amitfre15]

Fixed — the fence is now sized longer than the longest backtick run actually present in the text (per CommonMark), instead of a fixed triple-backtick fence.

[oblinder]

Not a bug: this PR adds one real correctness_e2e row from 2026-10-06. Is this row intentional? If not, please remove it, because it will show in the dashboard trend.

[Amitfre15]

Confirmed intentional, not a bug -- kept as-is, no change.

[oblinder]

Code review: 10 findings, most severe first. Items 1–5 are correctness bugs. Items 6–10 are cleanup. I did not run tests.

[oblinder]

1. Bug: the dashboard parser does not read the new fence. _render_field now writes a fence of 4 or more backticks when the value contains a backtick run. But parse_report in eval/dashboard/dashboard.py (open check near line 202, close check near line 174) finds a fence only when line.strip() == "```".

Failure: a Failure/Reason value contains a triple backtick. The parser does not see the 4-backtick fence, so it assigns an empty value, and the drill-down drops the fenced body. Also, when the outer fence has 3 backticks and an inner line is exactly a triple backtick, the parser closes the fence too early.

Fix: make the parser accept a fence of 3 or more backticks, and close the fence only on a line with the same backtick count.

[Amitfre15]

Fixed -- the fence is sized longer than the longest backtick run in the content (CommonMark), and parse_report now accepts a fence of 3-or-more backticks that closes only on an exact-length match (commit 1a4c37f).

[oblinder]

2. Bug: the heading from the LLM is not escaped. rec.heading goes into the Markdown as it is.

Failure: a heading with a newline or a leading -/# breaks the report structure. For example, 'Over-grant\n- **Precision:** 0.1' makes a bullet that dashboard._BULLET_RE gives to the last scenario entry. A heading in backticks (for example `correctness_prb:baseline:over_grant`) matches _ENTRY_RE and makes a false ScenarioEntry.

Fix: replace newlines with spaces and remove (or escape) backticks and leading Markdown characters before you write the heading.

[Amitfre15]

Fixed -- added _sanitize_recommendation_heading, called at the one ### write site: collapses every whitespace/line-boundary char to a single space and strips backticks before the heading is written (commit 1a4c37f).

[oblinder]

3. Bug: raw newlines go into the evidence. The mismatches value is joined with newlines (test_policy_pipeline_consistency.py:105). This breaks the "one line per case" evidence rule.

Failure: when a scenario has mismatches on 2 or more gates/runs, _case_prompt_line writes the second and later lines without an indent, so they look like new cases in the batched prompt. In the report, '\n'.join(rec.evidence) no longer shows which line belongs to which case.

Fix: join the mismatches with ; (or use _format_pairs), or use !r, as the perturbation path does.

[Amitfre15]

Fixed -- wrapped mismatches in !r, same convention the perturbation field already uses, so it stays one line (commit 1a4c37f).

[oblinder]

4. Bug: the evidence has no size limit. This builder (and the over/under-grant builders) puts every pair into one evidence string, and all strings go into one batched LLM request.

Failure: scale_total_corpus has a corpus of about 9M tokens. A recall of 0.99 there can mean hundreds or thousands of under-granted pairs. The single _draft_patterns call can then go over the model's context window. call_with_retry fails, and every case falls back to the default grouping, so the LLM analysis is lost for the full run.

Fix: limit the number of pairs in each evidence string (for example, the first N and then ... and K more).

[Amitfre15]

Fixed -- format_pairs now caps each gate at 20 pairs with a trailing "... and N more" (commit 1a4c37f).

[oblinder]

5. Bug: the classification is wrong on filtered runs. The fraction counts only the scenarios that ran.

Failure: pytest -m eval -k "consistent and baseline" runs 1 scenario, and that scenario is inconsistent. The fraction is 1/1, so the result is "appears random/widespread". The evidence then tells the LLM to recommend temperature=0, but the data shows only one scenario (the "clusters" case).

Fix: divide by _EXPECTED_CONSISTENCY_SCENARIO_COUNT. Or, when fewer scenarios ran than expected, report that the result is not conclusive.

[Amitfre15]

Fixed -- classifies against the full expected corpus size (_EXPECTED_CONSISTENCY_SCENARIO_COUNT), not however many scenarios ran this session; a filtered run now reports "not conclusive" instead of forcing a verdict (commit 1a4c37f).

[oblinder]

6. Cleanup: pairs appear twice. under_grant_cases lists both under_grants and incorrectly_denied. The docstring says that incorrectly_denied is normally a subset of under_grants, so the same pairs appear two times. This doubles the evidence text and the tokens sent to the LLM.

Fix: add only incorrectly_denied - under_grants.

[Amitfre15]

Fixed -- under_grant_cases now only adds the part of incorrectly_denied not already covered by under_grants (_pairs_not_in), so the same pair is never restated twice (commit 1a4c37f).

[oblinder]

7. Cleanup: this try/except does no work. raise_sanitized(err) always raises an LLMError subclass, and the code catches it at once and returns []. A plain except Exception: return [] does the same thing. With that change, readers do not have to read raise_sanitized to see that result is always assigned.

[Amitfre15]

Fixed -- simplified to a plain except Exception: return [] (commit 1a4c37f).

[oblinder]

8. Cleanup: the same loop exists two times. _collect_recommendation_evidence repeats the loop in _write_trend_log over _reports, and the same match of nodeid substrings against the same four marker dicts. If you add a marker or suite to one loop and not the other, it disappears from the trend log or from the recommendations, with no error. The docstring states the duplication but does not remove it.

Fix: put the classification of nodeid to suite/family into one shared helper.

[Amitfre15]

Fixed -- extracted _classify_nodeid/_NodeidClassification as the one shared nodeid -> suite/family lookup, used by both _write_trend_log and _collect_recommendation_evidence (commit 1a4c37f).

[oblinder]

9. Cleanup: the pair formatter exists two times. _format_pairs is a copy of eval.conftest._format_pairs_dict (conftest.py:285) with a different separator. The circular-import reason does not apply, because conftest already imports from recommendations.

Fix: keep one function with a sep parameter here, and import it in conftest.

[Amitfre15]

Fixed -- one shared format_pairs(sep=, limit=) in recommendations.py now; _format_pairs_dict in conftest.py delegates to it instead of keeping its own duplicate copy (commit 1a4c37f).

[oblinder]

10. Cleanup: the same list is passed two times. The same correctness_entries list goes in as over_grant and as under_grant, and build_recommendations takes them as two parameters. A caller can pass two different lists by mistake and get over/under findings that do not agree.

Fix: use one correctness= parameter that feeds both builders.

[Amitfre15]

Fixed -- build_recommendations now takes one correctness= parameter that feeds both over_grant_cases and under_grant_cases, instead of two always-identical over_grant=/under_grant= ones (commit 1a4c37f).

[oblinder]

Code review, third pass (head e971486).

All 10 findings from the previous review are fixed, and the offline suite passes (1107 passed). Thank you.

This pass found 10 new findings, but no crash bugs and no regressions. Items 1–4 make the new feature weaker. Items 5–8 are minor. Items 9–10 are design points that can go to a follow-up issue. These findings were not verified one by one, so some can be false positives.

[oblinder]

1. Worth fixing: the 20-pair limit can hide the real mismatch. sensitivity_cases (and invariance_cases, line 249) send the full expected and actual grant lists, and each list is cut at 20 pairs per gate.

Failure: a gate has 30 expected pairs, and the PRB drops pair #25 (sorted order). Both lists then show the same first 20 pairs plus ... and N more. The LLM and the fallback evidence cannot see which pair changed.

Fix: send the difference for each gate (expected - actual and actual - expected), not the full lists.

[Amitfre15]

Fixed -- sensitivity_cases/invariance_cases now send a diff (_grant_diff: lost/gained per gate) instead of the two full expected/actual lists, so a cap on either list can't hide which pair actually changed (commit 9f009c8).

[oblinder]

2. Worth fixing: some evidence still has no size limit. The pair limit applies only to pair lists. The mismatches text shown with !r, the perturbation policy text shown with !r, and the number of cases have no limit, and all of them go into one batched LLM request.

Failure: with a high PRB_CONSISTENCY_REPEATS or a long policy-text edit, the single _draft_patterns request can go over the context window. Then every case falls back, which is the failure the pair limit was added to prevent.

Fix: cut each evidence string at a maximum length (for example, the first N characters and then ... (truncated)).

[Amitfre15]

Fixed -- a new _truncate(text, limit=500) bounds the mismatches/perturbation evidence fields, applied as repr(_truncate(text)) (not _truncate(repr(text))) so the result stays a well-formed quoted string (commit 9f009c8).

[oblinder]

3. Worth fixing: the fallback text is not correct. The fallback body always says "LLM pattern analysis unavailable". Since the last fix, the fallback also runs when the LLM call worked but did not cover some case ids.

Failure: the LLM returns 3 valid patterns but omits 1 case. The report shows 3 LLM recommendations plus one "LLM pattern analysis unavailable" group, so a reader thinks the LLM call failed.

Fix: use a different text for cases that the LLM did not cover (for example, "not covered by the LLM pattern analysis").

[Amitfre15]

Fixed -- build_recommendations now distinguishes the two cases: "LLM pattern analysis unavailable" only when nothing survived at all, "Not covered by the LLM pattern analysis (other cases in this run were)" when some cases were (commit 9f009c8).

[oblinder]

4. Worth fixing: the same pairs appear twice. scale_mistake_cases still writes the full incorrectly_denied list next to under_grants. under_grant_cases now removes those pairs with _pairs_not_in, but this builder does not.

Failure: a 100-entity Scale run sends the same pairs twice in the largest evidence strings of the batched request.

Fix: use _pairs_not_in(incorrectly_denied, under_grants) here too.

[Amitfre15]

Fixed -- scale_mistake_cases now applies pairs_not_in(incorrectly_denied, under_grants) the same way under_grant_cases already does, and omits the clause entirely (not as "-> none") when nothing's left (commit 9f009c8).

[oblinder]

5. Minor: a case can appear under two recommendations. The code removes duplicate case ids inside one pattern, but not across patterns.

Failure: the LLM puts the same case id in two patterns. That case's evidence line then appears under two ### recommendations, which counts the failure two times and goes against the "distinct patterns" rule in the system prompt.

Fix: skip ids that are already in covered_ids.

[Amitfre15]

Fixed -- a case id already claimed by an earlier pattern is now skipped by every later pattern too (not just deduped within one pattern), so the same case can no longer appear under two different recommendations (commit 9f009c8).

[oblinder]

6. Minor: a failed render can leave a section that is not complete. _render_recommendations_section adds lines directly to the shared lines list. If a later step raises, the try/except in pytest_sessionfinish prints "omitting it", but the lines that were already added stay in the report.

Fix: render into a local list, and extend lines only after rendering succeeds.

[Amitfre15]

Fixed -- _render_recommendations_section now renders into a local list and extends the real report lines only once the whole section succeeds, so a failure partway through can no longer leave a half-written section in the report (commit 9f009c8).

[oblinder]

7. Minor: the LLM client libraries now load in every session. Because of this top-level import, every pytest session that loads eval/conftest.py loads langchain_core, pydantic, langchain_openai and tenacity. This includes the plain offline unit lane, because eval/ is in testpaths. This goes against the module's own comment that it "stays free of heavy imports". An import error in that stack would break collection of the whole suite. (Today the cost is small: the offline suite still runs in about 7 seconds.)

Fix: import build_recommendations inside _render_recommendations_section, and keep format_pairs in a small module with no LLM imports.

[Amitfre15]

Fixed -- format_pairs/pairs_not_in moved to a new eval/pairs.py with no LLM imports, and build_recommendations is now a local import inside _render_recommendations_section instead of a module-top one. This narrows the blast radius of a broken optional LLM dependency to this feature's own test module rather than eval/conftest.py's shared hooks -- it doesn't fully eliminate the import from a bare offline collection, since eval/test_recommendations.py itself still needs it to test _draft_patterns directly; documented that more precisely in both docstrings (commit 9f009c8).

[oblinder]

8. Minor: every eval run with failures calls the LLM, with no way to turn it off. pytest_sessionfinish makes a blocking LLM call (with retries) when at least one case fails. This includes -k debug runs.

Failure: a developer reruns one failing scenario. The end of the run waits up to max_retries × request_timeout plus backoff, and uses tokens.

Fix: control this with an env flag, or skip it for partial runs (the trend log already tags these as "partial").

[Amitfre15]

Fixed -- a new EVAL_RECOMMENDATIONS_LLM env var (default enabled, documented in docs/evaluation/README.md) lets _draft_patterns return [] immediately with no call attempted at all when set to 0/false/no (commit 9f009c8).

[oblinder]

9. Design: two parsers for the same nodeid format. _scenario_name_from_nodeid uses rindex('['), but eval/dashboard/dashboard.py uses _SCENARIO_RE (a regex that does not accept nested brackets) for the same job. For unusual param ids they give different results, so the recommendations and the dashboard can show the same test under different scenarios.

Fix: use one shared helper in both places.

[Amitfre15]

Fixed -- extracted a shared eval/nodeid.py (scenario_for_nodeid), used by both eval.conftest and eval.dashboard.dashboard instead of two independent parsers (commit 9f009c8).

[oblinder]

10. Design (follow-up): the fix is at the wrong level. The parser stops at the recommendations heading, and _sanitize_recommendation_heading removes backticks. Both are there only to keep the line-regex Markdown parser from reading LLM text as scenario data. Any section that is later added after the recommendations will be dropped with no warning, and each new free-form section needs another special case.

Suggestion for a follow-up issue: give the dashboard structured data (for example, a JSON file next to the report) and do not parse Markdown that contains LLM output.

[Amitfre15]

Noted for a follow-up issue, as suggested -- not addressed in this PR.

[oblinder]

Code review by Claude Code (high effort, 8 angles). This review found 10 possible problems. They were not verified separately, so please treat them as candidates. Inline comments are in severity order:

1. Scorer does not apply deny-overrides (correctness_scorer.py:104), attached at recommendations.py:188
2. Silent LLM failures in _draft_patterns
3. SCALE_CONCURRENCY README regression
4. Trend-log row from a branch run labelled regression
5. Case IDs sent as reprs, so returned IDs do not match
6. Wrong "not conclusive" label after one scenario error
7. No tests for the conftest glue
   8–10. Minor: duplicate guards, duplicate loop, repr() applied twice

🤖 Generated with Claude Code

[oblinder]

Scorer bug: deny-overrides is not applied (most severe). This line works around the problem, but the cause is in eval/correctness_scorer.py:104 (not in this diff): under_grants = frozenset(expected - granted) does not apply deny-overrides.

If a pair is expected, granted and denied, it counts as a true positive. Recall stays at 1.0 and the test passes, but OPA denies that pair at runtime. The docstring claim "incorrectly_denied is always a subset of under_grants" is not true for this case.

Example: the auditor splits a coarse scope into one approved and one best-effort-rejected sub-decision for the same expected (role, scope) pair. OPA denies the pair, but score_scenario gives recall=1.0, and the trend log records perfect recall. Only this recommendations section shows the miss.

Suggested fix: correct it in the scorer, for example under_grants = expected - (granted - denied). Then remove the special case here.

[oblinder]

LLM failures are silent. except Exception: return [] logs nothing. A failed LLM call, a schema rejection, or a code bug inside the try all look the same as the EVAL_RECOMMENDATIONS_LLM=0 opt-out.

Example: LLM_MODEL points to a model that does not support structured output. Each eval run then shows "LLM pattern analysis unavailable (no usable drafted pattern)" and gives no cause. The offline tests mock this seam, so they cannot find it.

Suggested fix: at minimum, print or log err before return [].

[oblinder]

Doc regression. Commit 4fad6fa put back the older, wrong SCALE_CONCURRENCY text. On main this row says that the variable controls only the PRB decision calls (total-corpus) and that the e2e Keycloak-admin calls run one at a time. eval/scale_prb.py agrees: SCALE_CONCURRENCY limits only the PRB ThreadPoolExecutor.

Possible result: a developer increases SCALE_CONCURRENCY to make e2e Keycloak provisioning faster, but it stays serial. Please restore the main text.

[oblinder]

Trend-log row from a branch run. This row (2026-10-06, correctness_e2e, precision 0.969, denial_precision 0.75) comes from a verification run on this feature branch, but it has run_type: "regression".

The trend log is append-only, and the dashboard charts it as history. After merge, everybody will see this dev run as a real regression data point in the correctness_e2e chart. Please remove this row from the PR, or label it as a non-regression run.

[oblinder]

Case IDs are sent as reprs. {case.case_id!r} sends case_id='correctness_prb:baseline:over_grant' with the quotes. But build_recommendations compares the returned IDs to the bare IDs with an exact match (cid in cases_by_id).

If the model copies the IDs "verbatim, from the input" with the quotes, no ID matches. The code drops all patterns, and the section shows "LLM pattern analysis unavailable" even though the LLM call worked.

Suggested fix: send bare IDs, or remove the quotes before the comparison.

[oblinder]

Wrong "not conclusive" label after one scenario error. entries contains only the scenarios that recorded a result. If one scenario fails before record_property (for example, orchestrate_prb raises), then len(entries) is 7, not 8.

Example: in a full 8-scenario run, one scenario errors and 5 of the other 7 disagree. The label is "not conclusive (7/8 ...)" and not "appears random/widespread", so the LLM gets no cluster-vs-random guidance for the run that needs it most.

Suggested fix: count errored scenarios as scored for this check, or compute the fraction over len(entries) when most scenarios are present.

[oblinder]

No tests for the conftest glue. The 23 new tests cover only the pure builders, and they call build_recommendations with hand-built input tuples. No test covers _collect_recommendation_evidence or _render_recommendations_section.

Example: someone renames a record_property key in a suite (such as sensitive), or changes the correctness gate to check only over_grants. A full finding category then disappears from the section, and no test fails. The try/except in pytest_sessionfinish also prints the error and continues, so a broken wiring shows only in a live report.

Suggested fix: add a test that fills _reports with fake reports that have user_properties, and assert on the rendered section.

[oblinder]

Minor: two guards for one problem. This function removes all backticks from LLM headings, so the dashboard does not parse them as fake ### ...`` scenario entries. But parse_report now stops at `## Improvement recommendations`, so it never reads these headings.

Result: a correct heading such as Over-interpreting `only` shows as "Over-interpreting only" and loses its formatting. Suggestion: keep the parser stop and remove this sanitizer (or keep only whitespace normalization).

[oblinder]

Minor: duplicate loop over _reports. This loop repeats _classify_nodeid and dict(report.user_properties) for each report that _write_trend_log (line ~845) processes in the same pytest_sessionfinish.

The docstring says this is deliberate, to keep the trend-log writer unchanged. But now each new suite or property gate must be added in two places to keep the trend log and the recommendations in step. _classify_nodeid was added to prevent this type of divergence. Suggestion (optional, can be a follow-up): use one pass that classifies each report once and gives the result to both writers.

[oblinder]

Minor: repr() applied twice. For the mechanical tier, the perturbation string already contains reprs (f"{edit_type}: {find!r} -> {replace!r}"). This line applies repr() again (the same pattern is used in invariance_cases).

Result: the evidence shows perturbation="negation: 'may' -> 'may not'", with backslash-escaped quotes if the text contains quotes. The LLM prompt and the report then show nested, escaped quotes. Suggestion: escape only the newlines (for example .replace("\n", "\\n")) and do not use a second repr().