feat: Enterprise Behavioral Testing certification check - #51
Conversation
GuyZivRH
left a comment
There was a problem hiding this comment.
Consolidated review: PR #51
Verdict: Request changes (not “approve with one fix”).
Library design and tests are strong (745 tests green; CI green), but enabling Certified without end-to-end producers is a product bug, and there are silent Compass/scorecard naming issues plus policy/process blockers.
Blocking
1. Check enabled; pipeline not wired → Certified broken in prod
ENTERPRISE_BEHAVIORAL_TESTING is in CERTIFIED_CHECKS and skill/agent/full profiles, with min-2 active sub-checks. This PR does not change pipeline/**. Harbor still runs treatment/control only; analyze never gets --edge-case-results-dir. _extract_behavioral_data typically only gets std_reward → min-2 fails → Certified always fails.
Not “backward compatible” for Certified outcomes (scaffold/validate without edge_cases/ are fine; Certified is not).
Fix: keep check out of CERTIFIED_CHECKS/profiles until Harbor build/eval + analyze are wired; or ship that wiring here; or temporarily min-1 with an explicit “partial” profile flag.
2. EdgeCaseGate breaks gate_name / policy_key convention
Docstring + every other gate: gate_name = category, policy_key = implementation. This gate sets both to "edge-case" while reading policy via get_gate_policy("behavioral").
Concrete fallout:
- Compass:
should_push_fact("edge-case")ignoresgates.behavioral.push_fact - WEIGHTED scorecard: weight looked up under
"edge-case", not"behavioral"
Fix: gate_name="behavioral", keep policy_key=self.name.
3. Missing edge trial output silently dropped
analyze_edge_cases does if not trials: continue, so empty/failed edge dirs never count as failures → inflated pass rates. Treat missing trial output as failure before calling this Certified-complete.
4. Submitter-owned edge_cases/ + min-2 is gameable
Only consistency + edge are extractable today. Author-written trivial edges can satisfy min-2. Need required packs / stricter rules (or don’t enable Certified yet).
Medium / follow-up
| Issue | Notes |
|---|---|
Stability / failure_mode / get_historical_variance |
Implemented & tested as library stubs, not wired in _extract_behavioral_data. Don’t oversell as staged product. |
EdgeCaseGate fail-open on [] / missing |
passed=True, score=1.0 — unlike quality BLOCK fail-closed; confusing vs certification min-2 fail |
| Consistency ignores absolute performance | Low mean + low variance still passes |
Dead DEFAULT_THRESHOLDS[…]=0.6 |
Unused after “pass = all_passed only” |
| Policy key split | is_enabled("edge-case") vs get_gate_policy("behavioral") (same smell as #3) |
Scaffold returns only (treatment, control) |
Edge dirs are log-only for callers |
| Threshold drift | Gate uses gates.behavioral.threshold; cert sub-check hardcodes DEFAULT_EDGE_CASE_PASS_THRESHOLD=0.5 — can diverge later |
| First-match gate scan in extract | Fine with one behavioral gate; fragile if more are added |
get_historical_variance untested |
Cheap to add when wiring stability |
| LLM new criteria | Opt-in via self.criteria — good for judge backward compat |
What looks good
- Min-2 +
all_passedas the only gate; composite score reporting-only (avoids “all pass / composite fail”). no_datasentinel so skipped sub-checks don’t false-fail.- Single path:
EdgeCaseGate→ extract → certification (no second parse ofreport.jsonfor cert). - Scaffold DRY helpers; optional
edge_cases/validation; score clamping. - Broad unit + scaffold→analyze→certify coverage; ruff clean on touched files; CI green.
Merge bar
- Strip Claude co-author trailers.
- Either wire Harbor build/eval + analyze for edge tasks end-to-end, or keep the check out of
CERTIFIED_CHECKS/profiles until that exists. - Fix
gate_name="behavioral". - Treat missing edge trial output as failure.
- Be explicit in PR text: scaffold/validate compat ≠ Certified outcome change; stability/failure_mode are stubs until extract + producers land.
Until (2)–(4) land, this should stay Request changes, not approve-with-nits.
Add the ENTERPRISE_BEHAVIORAL_TESTING check (Certified level) with four sub-checks: consistency (trial variance), edge case testing, long-running stability, and failure mode analysis. - Consistency check flags skills with high trial variance (std_reward > 0.3) - Edge case testing: validation, scaffolding, analysis, and new BEHAVIORAL gate type with EdgeCaseGate registry - Stability and failure mode sub-checks implemented and tested, staged for wiring when upstream data producers exist - Certification wiring: uncommented in CERTIFIED_CHECKS, updated profiles, weighted composite with minimum-2 sub-check coverage requirement - Scaffold refactored: extracted _write_rendered_templates and _copy_root_dirs - 55+ new tests including end-to-end scaffold-to-certify integration test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ition - Fix import ordering in monitor.py (ruff I001) - Fix line length in test_behavioral_checks.py (ruff E501) - Run ruff format on certification.py, edge_case.py, scaffold.py - Drop composite score threshold from behavioral testing pass condition: all_passed is the sole gate, score is for reporting only - Clamp scores with min(1.0, ...) to prevent > 1.0 on negative inputs - Remove unused threshold_overrides param from _compute_behavioral_testing_check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Keep ENTERPRISE_BEHAVIORAL_TESTING out of CERTIFIED_CHECKS and profiles until pipeline wiring exists (prevents Certified regression) - Restore gate_name='behavioral', policy_key=self.name convention in EdgeCaseGate (fixes Compass push and weighted scorecard lookups) - Treat missing edge case trial output as failure instead of silently skipping (prevents inflated pass rates) - Remove dead DEFAULT_THRESHOLDS entry for behavioral testing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Consistent with security gates which fail when scan files are missing in BLOCK mode. WARN mode still passes gracefully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Resolve conflicts with APPENG-5623 (operational policy) merge - Pass operational_policy_result in certification integration tests - Add tests verifying consistency and mean reward are independent: low mean + low variance passes consistency (quality caught elsewhere), high mean + high variance fails consistency (flaky behavior caught) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
386f3b0 to
cc7e3db
Compare
Pipeline wiring: - Add ase-eval-edge-cases step: runs separate ASE per edge case .md file - Add ase-aggregate-edge-cases step: produces per-edge-case pass/fail - Merge edge case results into report.json in analyze task - New scripts: generate_edge_case_evals.py, aggregate_edge_case_evals.py Certification: - Uncomment ENTERPRISE_BEHAVIORAL_TESTING in CERTIFIED_CHECKS - Enable in skill profile only (not agent/full per Guy's request) Cleanup: - Remove dead Harbor-based analyze_edge_cases() and --edge-case-results-dir - Simplify scaffold.py: removed Harbor edge case scaffolding since edge cases now run via ASE directly from the submission directory - Make EdgeCaseResult.summary optional for ASE-produced results - Add integration test: low-mean skill blocked by engine gate even when consistency passes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…yaml import - Corrupt grading.json now counts edge case as failed instead of skipping (prevents inflated pass rates) - Use shared DEFAULT_EDGE_CASE_PASS_THRESHOLD from certification.py instead of hardcoded 0.5 in aggregation script - Move yaml import to top level in generate_edge_case_evals.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GuyZivRH
left a comment
There was a problem hiding this comment.
PR #51 Review: Enterprise Behavioral Testing
Verdict: Request changes
PR: #51 | CI green | 6 commits | +2,026/−43
What looks good
- ASE-per-edge-case wiring (
ase-eval-edge-cases+aggregate_edge_case_evals.py+ merge intoreport.json) is the right shape gate_name="behavioral"/policy_key=self.namematches Compass/scorecard conventions- Fail-closed missing
report.jsonin BLOCK mode - Missing
grading.jsoncounted as failure (not silently skipped) - Composite score is reporting-only; per-sub-check thresholds gate pass/fail
- Strong unit coverage (813 lines) for sub-checks and certification integration
- Scaffold no longer pretends Harbor owns edge cases
- Clean architecture with new
GateType.BEHAVIORALfollowing existing gate registry pattern
Blocking (must fix before merge)
1. False backward-compatibility claim
PR says: "Backward compatible — submissions without edge_cases/ work exactly as before."
This is incorrect for skill Certified level:
ENTERPRISE_BEHAVIORAL_TESTINGis enabled in theskillCertified profile- Check requires min 2 sub-checks with real data
- Only two producers exist today:
| Sub-check | Wired? |
|-----------|--------|
| consistency (std_reward) | Yes |
| edge_case | Only ifedge_cases/exists and ASE ran |
| stability | No (TODO) |
| failure_mode | No (TODO) |
Withoutedge_cases/, a submission getsn_active == 1→ "Insufficient behavioral coverage… need at least 2" → Certified always fails.
Fix options: - Keep the check commented out of Certified/skill until more producers exist or
edge_cases/is a required asset - Drop min-2 for MVP (consistency alone can pass)
- Make
edge_cases/a required Certified asset and document this as an intentional breaking change
2. Edge-case ASE steps skip eval-engine=ase
The condition only allows harbor / both:
if [ "$EVAL_ENGINE" != "harbor" ] && [ "$EVAL_ENGINE" != "both" ]; then
echo "Skipping edge case eval (eval-engine=$EVAL_ENGINE)"
exit 0
fiBut the implementation is ASE-based. Pure ase runs never evaluate edge cases → same Certified failure due to min-2.
Fix: Run whenever edge_cases/ exists, at minimum for ase and both.
Major (should fix)
3. Stale profile comments
full and agent profiles say "Implemented but not wired into pipeline yet" while skill has it enabled. Update comments to reflect current state.
4. Gate policy key mismatch
is_enabled(behavioral_gate.name)checks"edge-case"EdgeCaseGate.evaluate()callsget_gate_policy("behavioral")
Disabling viagates.behavioralvsgates.edge-casebehaves differently. Align with security/quality convention (one key for both enable check and evaluate).
5. Runtime npm install --global agent-skills-eval on :latest
Same reproducibility/airgap concern as other pipeline steps. Prefer a pinned ASE image or digest for production.
Minor (non-blocking)
- Unquoted variable:
for MD_FILE in $MD_FILESbreaks on filenames with spaces || trueon ASE: Hides infra failures (aggregate marks fail — acceptable but noisy)- Empty
edge_case_results: Returns pass score 1.0 in BLOCK mode (scorecard noise) - Generic ASE assertions: Hardcoded defaults are weak for "behavioral" claims — fine for MVP
- Stability/failure_mode stubs: OK if documented as future work, not if marketed as Certified coverage
Required before merge
- Fix Certified enablement vs min-2 / optional
edge_cases/(product decision + code alignment) - Run edge-case ASE for
eval-engine=ase, not onlyharbor/both
Multi-line Python block caused YAML parse error due to colons being interpreted as YAML mappings. Converted to one-liner matching existing enrichment step patterns. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Run edge case ASE for eval-engine=ase and both, not just harbor - Update agent/full profile comments: 'Enabled for skill profile only' - Fix inline Python YAML parse error (one-liner for compatibility) - PR description updated to document Certified breaking change Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The ase-eval-edge-cases step uses a Node.js image which doesn't have python. Generate evals.json inline using node for JSON escaping. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
492052d to
9403a74
Compare
|
Validated end-to-end on the cluster. With edge cases:
|
Summary
Implements the
ENTERPRISE_BEHAVIORAL_TESTINGcertification check (Certified level), which validates that skills behave correctly under stress, edge cases, and unusual conditions — not just the happy path.std_reward > 0.3), using data already captured in every evaluation runedge_cases/submission directory with per-edge-case ASE evaluation,EdgeCaseGatewith newBEHAVIORALgate typeget_historical_variance()and_check_stability()implemented and tested, staged for wiring when upstream data producer existsfailure_handlinganduncertainty_acknowledgmentcriteria added toLLMJudgeVerifier,_check_failure_mode()readyCERTIFIED_CHECKSandskillprofile, weighted composite with minimum-2 sub-check coverage requirementBreaking change: Certified level for skill profile
This PR intentionally raises the bar for Certified level in the skill profile. Skills now require an
edge_cases/directory with behavioral edge case tests to reach Certified. Without edge cases, the behavioral testing check fails due to the minimum-2 sub-check requirement (only consistency has data).edge_cases/will fail the Certified check — this is the intended behaviorDesign decisions
all_passedonly — each sub-check enforces its own threshold. The composite score is a weighted average for reporting, not a gate.EdgeCaseGateis the source of truth; certification consumes its result rather than re-readingreport.json.