fix(report): red-team report fails closed — no green for unanalyzed runs, no vanished broken runs, no compromise filed as held - #890
Conversation
WalkthroughRed-team reporting now distinguishes held, compromised, errored, and analysis-failed runs. JavaScript and Python use a shared early-exit marker and status vocabulary. Dashboard risk classification applies fail-closed fallbacks and displays analyzer failures. ChangesRed-team report classification
Sequence Diagram(s)sequenceDiagram
participant RedTeamAgent
participant JudgeAgent
participant saveRedTeamReport
participant Dashboard
RedTeamAgent->>saveRedTeamReport: report outcome and early-exit reasoning
JudgeAgent->>saveRedTeamReport: infrastructure error metadata
saveRedTeamReport->>saveRedTeamReport: normalize status and analyze report
saveRedTeamReport->>Dashboard: saved report with risk and analysis_failed
Dashboard->>Dashboard: render status and risk classification
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
javascript/src/red-team-report.ts (1)
126-149: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist errored runs as unsuccessful and retain the error reason.
When
opts.errorexists withopts.result.success === true, Line 148 writessuccess: truewhile Lines 131-135 writestatus: "errored". Line 149 can also hiderunErrorbehind existing result reasoning. Persistsuccess: falsewheneverrunErrorexists, and include the error inreasoning.Add a test with
result.success: trueanderror: "connection refused".Proposed fix
const status = runError ? "errored" : opts.result.success && !objectiveAchieved ? "held" : "broke"; + const reasoning = runError + ? `ERROR: ${runError}${ + opts.result.reasoning ? `\n\n${opts.result.reasoning}` : "" + }` + : opts.result.reasoning || ""; + const messages = (opts.result.messages || []).map(serializeMessage); @@ - success: Boolean(opts.result.success) && !objectiveAchieved, - reasoning: opts.result.reasoning || (runError ? `ERROR: ${runError}` : ""), + success: Boolean(opts.result.success) && !objectiveAchieved && !runError, + reasoning,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/src/red-team-report.ts` around lines 126 - 149, Update the payload construction in the red-team report flow so success is false whenever runError exists, even if opts.result.success is true. Ensure reasoning includes the runError rather than allowing existing result reasoning to hide it, while preserving current behavior for non-error results. Add coverage for a successful result with error "connection refused" verifying status, success, and reasoning.
🧹 Nitpick comments (2)
python/scenario/red_team_agent.py (1)
38-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark outcome and risk constants as immutable. These values define report classification behavior and should not be reassigned.
python/scenario/red_team_agent.py#L38-L43: declareEARLY_EXIT_OBJECTIVE_PREFIXasFinal[str].python/scenario/report/_risk.py#L12-L37: declare immutable classification constants asFinal; use immutable containers where mutation is not required.As per coding guidelines: “Use
Finalfor constants that should not be reassigned in Python”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/scenario/red_team_agent.py` around lines 38 - 43, Mark EARLY_EXIT_OBJECTIVE_PREFIX in python/scenario/red_team_agent.py (lines 38-43) as Final[str]. In python/scenario/report/_risk.py (lines 12-37), annotate all immutable outcome and risk classification constants with Final and replace mutable containers with immutable equivalents where mutation is unnecessary.Source: Coding guidelines
javascript/src/__tests__/red-team-report.unit.test.ts (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the test execution requirements.
Add the repository test command, the Vitest dependency, coverage expectations, and a short
saveRedTeamReporttest example. The current header only documents behavior.As per coding guidelines: “Document testing requirements explaining how to run tests, test coverage requirements, test dependencies, and providing test examples”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/src/__tests__/red-team-report.unit.test.ts` around lines 1 - 9, Expand the header documentation for the red-team report tests to include the repository test command, the Vitest dependency, expected coverage requirements, and a concise saveRedTeamReport test example. Keep the existing behavioral requirements intact and limit changes to documenting test execution and expectations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/scenario/report/_save.py`:
- Around line 281-322: Define a typed Analysis TypedDict in _save.py and use it
for _analyze()’s return value and the analysis variable, validating analyzer
JSON with explicit isinstance/type-narrowing checks before assigning persisted
fields such as break_severity and failing_turn_index. Update _risk.py lines
40-89 to accept the typed report shape or validate legacy JSON into it before
normalization and status classification; cover both listed sites and avoid
unparameterized dict or Any usage.
In `@python/tests/test_redteam_report_fail_closed.py`:
- Around line 169-175: Extend
test_agent_early_exit_message_carries_the_shared_prefix to run the early-exit
step produced by marathon_script() using a fake executor, then capture the
succeed() call and assert its reasoning starts with EARLY_EXIT_OBJECTIVE_PREFIX.
Preserve the existing check_early_exit() setup and assertion while validating
the emitted message.
---
Outside diff comments:
In `@javascript/src/red-team-report.ts`:
- Around line 126-149: Update the payload construction in the red-team report
flow so success is false whenever runError exists, even if opts.result.success
is true. Ensure reasoning includes the runError rather than allowing existing
result reasoning to hide it, while preserving current behavior for non-error
results. Add coverage for a successful result with error "connection refused"
verifying status, success, and reasoning.
---
Nitpick comments:
In `@javascript/src/__tests__/red-team-report.unit.test.ts`:
- Around line 1-9: Expand the header documentation for the red-team report tests
to include the repository test command, the Vitest dependency, expected coverage
requirements, and a concise saveRedTeamReport test example. Keep the existing
behavioral requirements intact and limit changes to documenting test execution
and expectations.
In `@python/scenario/red_team_agent.py`:
- Around line 38-43: Mark EARLY_EXIT_OBJECTIVE_PREFIX in
python/scenario/red_team_agent.py (lines 38-43) as Final[str]. In
python/scenario/report/_risk.py (lines 12-37), annotate all immutable outcome
and risk classification constants with Final and replace mutable containers with
immutable equivalents where mutation is unnecessary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62445dfb-db34-44bf-8241-32f146436651
📒 Files selected for processing (10)
javascript/src/__tests__/red-team-report.unit.test.tsjavascript/src/agents/judge/interfaces/judge-result.interface.tsjavascript/src/agents/judge/judge-agent.tsjavascript/src/agents/red-team/red-team-agent.tsjavascript/src/red-team-report.tspython/scenario/red_team_agent.pypython/scenario/report/_risk.pypython/scenario/report/_save.pypython/scenario/report/app.pypython/tests/test_redteam_report_fail_closed.py
Ruthless review — findings and fixesIndependent review agent traced this diff with executed probes (not code-reading); findings below, all now addressed in the three follow-up commits. P1 (introduced-on-top-of-inherited, FIXED): Python's judge infra failure still filed as a real security break. P2 (introduced, FIXED): a HELD run whose analyzer call failed rendered as a MEDIUM-risk finding ( P2 (introduced-as-incomplete, FIXED): the early-exit re-bucket never reached the risk number — an objective-achieved run whose analyzer said P3s (FIXED): P3 (noted, not changed): Also verified clean by the reviewer: no Python or JS import cycles from the new shared constant/module (executed under Post-fix verification: 16/16 fail-closed tests (5 new), full Python suite 1,242 passed, pyright 0 errors. Blessed. 🙏 The report now fails closed on every path — no compromise wears green, no broken run vanishes, and the judge's silence is filed as silence, not as a verdict.
Traced writer → saved JSON → dashboard in both languages with executed probes; verdict: ship it. Residual: the |
… review P1 ScenarioResult gains an optional error field (mirroring the JS SDK); the judge's discovery-non-convergence return sets it, and the writer files any result carrying it as errored — even with a full transcript — instead of fabricating a 'partial break' compromise. Review finding on #890.
…egacy early-exit reports floor at partial Review P2s on #890: severity is analyzer-produced, so escalating a HELD run to the default 'medium' on analyzer failure carried no information — the ANALYSIS FAILED chip alone signals the uncertainty. And a legacy objective-achieved report whose stored break_severity is 'none' now floors to 'partial' at read time so the risk number agrees with the COMPROMISED card.
…ignificant; aggregate prompt uses shared status; drop dead import Review P2/P3s on #890: the early exit is the strongest evidence available, so an analyzer opinion of 'none' no longer puts a confirmed compromise back at RISK NONE; _aggregate's findings block goes through _risk._status so legacy 'broken'/early-exit reports are not described as [held] to the fix-clustering model; unused _BREAK_ORDER import removed from app.py.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/tests/test_redteam_report_fail_closed.py (1)
178-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate both
tmp_pathfixture parameters.Add
tmp_path: Pathto both test methods. Do not add-> Noneto these test methods.As per coding guidelines, “Always use explicit type annotations for function parameters, return types, and class attributes in Python.” Based on learnings, “leave pytest test functions unannotated (do not add
-> None).”Also applies to: 225-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/tests/test_redteam_report_fail_closed.py` at line 178, Annotate the tmp_path parameter with Path in both test methods, including test_result_error_files_as_errored_not_broke and the additional test identified by the review. Do not add return annotations such as -> None to either pytest test method.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/scenario/types.py`:
- Around line 272-276: Update the ScenarioResult handling to propagate a set
result.error as an infrastructure failure: serialize error=result.error and emit
ScenarioRunFinishedEventStatus.ERROR instead of mapping it to FAILED. Preserve
the existing behavior for results without an error.
In `@python/tests/test_redteam_report_fail_closed.py`:
- Around line 195-205: Replace the source-text assertions in
test_python_judge_sets_error_on_discovery_non_convergence with an actual
_parse_response exercise using a response containing a leaked discovery tool
call. Assert that the returned ScenarioResult has success set to False and a
non-empty error value, ensuring the discovery non-convergence branch in
JudgeAgent is executed and validated.
---
Nitpick comments:
In `@python/tests/test_redteam_report_fail_closed.py`:
- Line 178: Annotate the tmp_path parameter with Path in both test methods,
including test_result_error_files_as_errored_not_broke and the additional test
identified by the review. Do not add return annotations such as -> None to
either pytest test method.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 970d2707-2120-487e-9c8f-069d43d200f6
📒 Files selected for processing (7)
python/scenario/judge_agent.pypython/scenario/report/_aggregate.pypython/scenario/report/_risk.pypython/scenario/report/_save.pypython/scenario/report/app.pypython/scenario/types.pypython/tests/test_redteam_report_fail_closed.py
💤 Files with no reviewable changes (1)
- python/scenario/report/app.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/scenario/report/_save.py
…nd objective-achieved exits can no longer render green Three fail-open paths in the Python writer/dashboard (#888): - An analyzer failure (or an unrecognized break_severity) kept the default 'none', so a compromised run whose analysis call 429'd rendered 'RISK - NONE' in green. The writer now flags analysis_failed, derives a status-based break-severity floor (broke -> partial, never none), and the dashboard renders an ANALYSIS FAILED chip with the same severity-fallback urgency as an errored run. - An early exit because the ATTACK achieved its objective was filed as held ('what worked') — succeed() only ends the script. Such runs now file as broke with success false, keyed on a shared EARLY_EXIT_OBJECTIVE_PREFIX marker; the dashboard re-buckets legacy reports by the same marker. - Status/risk classification moved to scenario/report/_risk.py (pure, no Streamlit) so these rules are testable; _status also normalizes the JS writer's legacy 'broken' vocabulary to 'broke' so those runs stop vanishing from every tile. Part of #888
…ng verdicts - status 'broke', never the JS-only 'broken' that matched none of the dashboard's buckets and silently dropped broken runs from every tile; - judge infrastructure failures (discovery budget exhausted, unknown or missing tool call) now carry JudgeResult.error, and the writer files any errored result as 'errored' instead of a significant security break; - break_severity is written as '' when no analyzer has spoken, making the dashboard's status-based fallback reachable (broke -> partial) instead of filing every non-success as 'significant'; - an early exit because the attack achieved its objective files as broke with success false, sharing EARLY_EXIT_OBJECTIVE_PREFIX with the agent. Part of #888
… review P1 ScenarioResult gains an optional error field (mirroring the JS SDK); the judge's discovery-non-convergence return sets it, and the writer files any result carrying it as errored — even with a full transcript — instead of fabricating a 'partial break' compromise. Review finding on #890.
…egacy early-exit reports floor at partial Review P2s on #890: severity is analyzer-produced, so escalating a HELD run to the default 'medium' on analyzer failure carried no information — the ANALYSIS FAILED chip alone signals the uncertainty. And a legacy objective-achieved report whose stored break_severity is 'none' now floors to 'partial' at read time so the risk number agrees with the COMPROMISED card.
…ignificant; aggregate prompt uses shared status; drop dead import Review P2/P3s on #890: the early exit is the strongest evidence available, so an analyzer opinion of 'none' no longer puts a confirmed compromise back at RISK NONE; _aggregate's findings block goes through _risk._status so legacy 'broken'/early-exit reports are not described as [held] to the fix-clustering model; unused _BREAK_ORDER import removed from app.py.
… FAILED; review follow-ups CodeRabbit: the Python executor mapped a judge-infrastructure failure (result.error set) to a FAILED run-finished event — the platform filed a broken judge as a failed simulation, the same fabrication this PR removes from the report. Status derivation moves to _run_finished_status (error -> ERROR) and the error message rides the results payload. The weak inspect.getsource judge test now drives _parse_response with a leaked discovery tool call, and the early-exit test executes the marathon step against a fake executor asserting the shared reasoning prefix. Unused pytest import dropped.
…on isawaitable marathon_script always generates the early-exit check as a coroutine function, so the isawaitable branch was dead and left a bare `await maybe` that static analysis reads as a statement with no effect. Narrow the ScriptStep union with a cast and await unconditionally.
a4d3585 to
06f5ad2
Compare
Human Review BriefMode: Deep Review. Closes #888 ( Decisions being ratified
Must Check
Ask Author
Production riskThis is a security document that people act on. The change is the right direction, and its own failure mode is now over-reporting rather than under-reporting, which is the correct place to put the error. On the CI signalThe four red checks are from a run on 11 August that hit |
|
Automated low-risk assessment This PR was evaluated against the repository's Low-Risk Pull Requests procedure and does not qualify as low risk.
This PR requires a manual review before merging. |
langwatch-agent
left a comment
There was a problem hiding this comment.
Two P2 findings in the current head; details are inline.
LangWatch-Review: verdict=findings sha=06f5ad2a8549baa0ac55241253e44300c0d33a6d p0=0 p1=0 p2=2 p3=0
| status, | ||
| success: Boolean(opts.result.success), | ||
| reasoning: opts.result.reasoning || (opts.error ? `ERROR: ${opts.error}` : ""), | ||
| success: Boolean(opts.result.success) && !objectiveAchieved, |
There was a problem hiding this comment.
[P2] Make an errored report unsuccessful in its persisted contract.
runError makes status errored, but it is not part of this expression. A caller can therefore persist { status: "errored", success: true }; line 149 can also retain a normal judge rationale and omit the error entirely. The dashboard currently keys off status, but other report readers (and the raw JSON consumers this shared format is for) can reasonably use success and get the exact success/error contradiction this change is intended to remove. Include !runError in success, make the persisted reasoning lead with the error (preserving the original rationale after it if useful), and cover a successful result carrying an error.
| status = r.get("status") or ("held" if r.get("success") else "broke") | ||
| if status == "broken": | ||
| return "broke" | ||
| if status == "held" and str(r.get("reasoning") or "").startswith( |
There was a problem hiding this comment.
[P2] Persist the early-exit outcome instead of using prose as the current-data discriminator.
This exact prefix is separately defined in the Python and JavaScript runtimes, while the dashboard uses only the Python value to reinterpret reports from both writers. The tests only prove each runtime agrees with its own local constant, so a harmless wording edit in either writer silently turns future objective-achieved reports back into held in the dashboard (and an unrelated manual succeed() reason beginning with the phrase is falsely reclassified). Store an explicit objective_achieved/outcome field in new report JSON and classify from that; retain this prefix check solely as the legacy migration fallback. That makes the retrospective correction stable across SDK releases.
drewdrewthis
left a comment
There was a problem hiding this comment.
Third-party review (review-clerk). Two-pass review done; tests verified locally (17/17 pass; mutation-checked that reverting the objective-achieved override fails 2 of the fail-closed tests, confirming they're load-bearing, not vacuous). CI on head 06f5ad2 genuinely ran and exercised this change (test (3.12), python-complete, javascript-complete, ci-checks (24.x) all SUCCESS; no path-filter or step-skip vacuity observed on the gating jobs). One blocking correctness finding below; verdict comment has full detail.
| # (attack score >= threshold for N consecutive turns) — an analyzer | ||
| # opinion of "none" must not put a confirmed compromise back at | ||
| # RISK NONE (#888). | ||
| if ( |
There was a problem hiding this comment.
[correctness] Objective-achieved significant floor is unreachable when the analyzer raises.
The two floor blocks run in a fixed order and the first one silently consumes the sentinel the second one needs:
- Lines 326-330: if
break_severityis not a recognized value (this includes the""default when the analyzer raised an exception), it floors to"partial" if status == "broke" else "none". - Lines 336-341: the objective-achieved floor to
"significant"only fires whenbreak_severity in {"", "none"}— but block 1 has already turned""into"partial"by this point, so this branch can never see""for abrokerun.
Concretely: an objective-achieved run (attack succeeded, succeed() reasoning carries EARLY_EXIT_OBJECTIVE_PREFIX) whose analyzer call raises (429, timeout, etc.) ends up break_severity="partial", not the "significant" the PR's own comment above (lines 332-335) says is required — "the strongest evidence available ... must not put a confirmed compromise back at RISK NONE" (or, implicitly, below significant). Verified locally: patching litellm.completion to raise on an objective-achieved result yields status="broke", break_severity="partial", analysis_failed=True — same shape as an ordinary rate-limited compromise, not the elevated floor this code block exists to guarantee.
The existing test TestEarlyExitBreakSeverityFloor::test_new_reports_floor_at_significant only covers the analyzer succeeding with an explicit "none" opinion — it doesn't hit the exception path, so this gap has no regression test today.
Not a fail-open bug (status is still correctly "broke", never "held"), but it does violate the PR's own documented invariant for this specific combination, silently downgrading urgency on the exact class of run (objective-achieved + analyzer failure) this PR set out to floor highest.
Suggested fix: move the objective-achieved elevation before the generic fallback, or change its guard to also match the pre-fallback "" sentinel (e.g. check analysis_failed instead of break_severity in {"", "none"}).
Review verdict: NOT-READYReviewed at: CI positively verified on this SHA (not accepted on the ci-green label alone): Blocking — must resolve before this PR is doneEach item below also exists as a resolvable inline thread (linked). Resolve the thread to clear it.
Non-blocking (Decide / New Issue)
Verdict is prose, not a GitHub approval. Scope: review findings only — READY means no unresolved blocking review threads at this SHA. It is not a merge-readiness signal. |
For humans
The red-team report is a security document, and it was failing open: a run whose analysis crashed rendered as green "no risk", runs written by the JavaScript SDK could vanish from the dashboard tiles because of a one-word vocabulary mismatch, judge infrastructure failures were filed as significant security breaks, and — worst — a run that ended early because the attack succeeded was filed under "Attacks Held — What Worked". Every one of these now points the safe way.
Why
Four independent fail-open paths in the red-team report pipeline (writer → saved JSON → Streamlit dashboard), each of which misreports exactly the runs a security report exists to surface.
Closes #888
What changed
Python writer + dashboard
break_severity) now setsanalysis_failed: true, derives a status-based break-severity floor (a compromised run readspartial, never the greennone), and the dashboard renders an "ANALYSIS FAILED" chip with the same severity-fallback urgency as an errored run.brokewithsuccess: false—succeed()only ends the script; the defense did not hold. A sharedEARLY_EXIT_OBJECTIVE_PREFIXmarker (mirrored in both languages) keys the classification, and the dashboard re-buckets legacy held-with-that-reasoning reports the same way, so "What Worked" can never contain an objective-achieved run.scenario/report/_risk.py(pure, no Streamlit) so the rules are unit-testable;_statusalso normalizes the legacy JS"broken"to"broke".JavaScript writer + judge
"broke", never"broken"— JS-written broken runs stop vanishing from the Held/Compromised/Errored tiles.JudgeResultgains an optionalerrorfield, set on the judge's three infrastructure failures (discovery budget exhausted, unknown tool call, no tool call); the writer files any errored result aserrored, not a significant break. The judge-inconclusive path is deliberately untouched — that's Judge ends the run mid-conversation with finish_test(verdict: inconclusive), reported as FAILED — looks like the user simulator stopped responding #886/fix(judge): treat an unforced inconclusive finish_test as continue instead of failing the run #889.break_severityis written as""when no analyzer has spoken, making the dashboard's status-based fallback reachable instead of filing every non-success as"significant".Test plan
Test-first (both new test files failed before the fixes):
python/tests/test_redteam_report_fail_closed.py— 17 tests: raising analyzer on broke/held runs, unrecognized break_severity, deliberateanalyze=Falseis not an analysis failure, compound risk of an analysis-failed run, legacy"broken"normalization, early-exit filing and legacy re-bucketing, errored runs keep no verdict, and the review follow-ups (Python judge infra failure files as errored, held-with-failed-analysis keeps the matrix risk, objective-achieved severity floors).javascript/src/__tests__/red-team-report.unit.test.ts— 6 tests: shared vocabulary, errored separation (run error + judge infra error on the result), no inventedsignificant, early-exit files as broke.pyright0 errors;tsc --noEmitclean; eslint clean on changed files.Anything surprising?
"broken"— per the issue's note,app.py's status lookup was left keyed on"broke"."broken", and held-with-objective-achieved reasoning) at read time.Review follow-ups (post-review commits)
ScenarioResultgains the same optionalerrorfield as JS; the judge's discovery-non-convergence return sets it, and the writer files such results aserrored(review P1 — Python judge breakdowns filed as fabricated "partial breaks").ERROR(notFAILED) on the platform run-finished event whenresult.erroris set, and serializes the error in the results payload — a broken judge no longer files as a failed simulation.significant(new) /partial(legacy read); aggregation prompt uses the shared status normalization.