Skip to content

Fix audit-framework bugs and add regression tests#25

Merged
SushantGautam merged 1 commit into
mainfrom
fix/audit-bugs-2026-06
Jun 16, 2026
Merged

Fix audit-framework bugs and add regression tests#25
SushantGautam merged 1 commit into
mainfrom
fix/audit-bugs-2026-06

Conversation

@kelkalot

Copy link
Copy Markdown
Owner

Summary

Fixes a batch of bugs surfaced in a review of the framework. Each is isolated and covered by tests in the new tests/test_bugfixes.py (54 tests). Full suite: 352 passed, 1 skipped.

Bugs fixed

1. Score-based judges could never emit their documented output. helpfulness, factuality, and harm declared only a human-readable output_schema. Under the default json_format=True, model_auditor.py reads response_schema and falls back to the severity schema when it's absent — so the provider forced severity-shaped JSON and silently overrode each judge's score/category prompt. Each judge now declares a matching response_schema (consistent with abstention/binary_abstention).

2. Path traversal + timing-unsafe secret in the visualization server. The containment check in server.py was full_path.startswith(RESULTS_DIR), which a sibling directory (../results_private/x.json) or a symlink could bypass. Replaced with a resolved-path (Path.resolve()) containment check. The secret check now uses secrets.compare_digest over UTF-8 bytes (constant-time; non-ASCII tokens/secrets give a clean 401 instead of a 500).

3. CrossJudgeExperiment dropped auditor credentials. Only model/provider were forwarded from auditor_models; api_key and base_url are now passed through to the underlying AuditExperiment.

4. Duplicate scenario names corrupted stability statistics. Four ung scenarios each shared a name with a distinct scenario; per-scenario stats are keyed by name, so they collapsed. Renamed to be unique (no deletions — pack size still 1000), added a duplicate_scenario_names() helper, and a warning in the stability builder when duplicates occur.

5. Version mismatch. __init__.py hard-coded 0.1.0 while pyproject.toml said 0.1.7. __version__ is now single-sourced from package metadata with a pinned fallback; fixed the broken ModelAuditor(...) docstring example and a stray indent in pyproject.toml.

Robustness improvements (also flagged in review)

  • run_async resilience: one failing scenario no longer aborts the whole batch — it records an ERROR result. The resume cache now skips ERROR-bearing runs so transient failures are retried instead of being baked into the aggregates. Progress bars reconcile to 100% on failure.
  • strip_thinking: keeps content preceding an unclosed <think> tag instead of blanking the entire response (a pure dangling tag still returns "").
  • compare_judges: adds n_compared (the genuine overlap count) alongside n_total.

Review

The diff was put through an adversarial multi-lens review (correctness, regressions, security completeness, test adequacy, missed sibling sites). All confirmed findings — the resume-cache interaction, the compare_digest non-ASCII crash, progress-bar reconciliation, and two test-coverage gaps — were folded into this PR.

Testing

  • pytest tests/ → 352 passed, 1 skipped
  • New tests/test_bugfixes.py exercises every change, including path-traversal/symlink escapes, non-ASCII secrets, resume-retry of ERROR runs, and the version/pyproject single-sourcing guard.

Addresses a batch of bugs found in a framework review:

- Judges: helpfulness, factuality, and harm declared only a human-readable
  output_schema, so under json_format=True the auditor forced the default
  severity schema onto them and they could never emit their documented
  score/category output. Each now declares a machine-readable response_schema.

- Visualization server: the path check was a string-prefix test that a
  sibling dir ("../results_private/x") or a symlink could bypass. Replaced
  with a resolved-path containment check. Secret comparison is now
  constant-time (secrets.compare_digest over UTF-8 bytes, so non-ASCII
  tokens/secrets yield a clean 401 instead of a 500).

- CrossJudgeExperiment dropped the auditor api_key and base_url; both are
  now forwarded to the underlying AuditExperiment.

- ung pack: four scenarios shared a name with a distinct scenario, which
  collapsed per-scenario stability statistics. Renamed to be unique; added a
  duplicate_scenario_names() helper and a warning in the stability builder.

- Version was hard-coded to 0.1.0 in __init__ while pyproject said 0.1.7.
  __version__ is now single-sourced from package metadata with a pinned
  fallback. Fixed the broken ModelAuditor docstring example and a stray
  indentation in pyproject.toml.

- run_async now records an ERROR result per failed scenario instead of
  aborting the whole batch; the resume cache skips ERROR-bearing runs so
  transient failures are retried. Progress bars reconcile to 100% on failure.

- strip_thinking keeps content preceding an unclosed <think> tag instead of
  blanking the whole response.

- compare_judges reports n_compared (the genuine overlap count) alongside
  n_total.

Adds tests/test_bugfixes.py (54 tests) covering every change. Full suite:
352 passed, 1 skipped.
@kelkalot
kelkalot requested a review from SushantGautam June 11, 2026 05:20

@avalyset avalyset left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Author of #24 here — confirming these two touch points preserve the CrossJudgeExperiment contract.

The credentials passthrough is the fix #24 should have shipped with: auditor_models was meant to carry full per-judge auditor config, and silently dropping api_key/base_url fell back to the judge's own credentials. The .get(...) accessors keep the None default for callers who pass only model/provider, so existing behavior is unchanged.

On compare_judges: n_total was already len(report_a.per_scenario), so the docstring is now just accurate and no existing value moves. n_compared is the field that was actually missing — when the two judges don't cover identical scenario sets, using n_total as the denominator inflates it with results_a-only scenarios that were never compared, understating the shift rate. The overlap count is the right denominator.

Both read as bug fixes against #24's intent, not contract changes. Nothing blocking from my side.

@SushantGautam SushantGautam left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks cool

@SushantGautam
SushantGautam merged commit 8d51e50 into main Jun 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants