fix: create configs/safety.yaml with sensible defaults - #100
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesSafety configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 3
🤖 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 `@configs/safety.yaml`:
- Around line 11-35: The edit-scope configuration in the safety YAML does not
match the schema consumed by SafetyIntegration. Update the edit_scope keys and
value types to use allowed_patterns as a mapping, and connect the enabled
setting to the top-level enforcement flags expected by SafetyIntegration,
preserving the existing forbidden paths and directories. Ensure the resulting
immutable configuration is read instead of triggering validator defaults.
- Around line 42-62: Expand the metric_thresholds configuration used by
RegressionDetector to include the built-in thresholds for every
default-monitored metric, especially accuracy and error_rate. Preserve the
existing thresholds for duration_sec, memory_mb, execution_time, success_rate,
pass_rate, and correctness, and ensure the supplied map is complete rather than
omitting default entries.
In `@TODO.md`:
- Around line 308-311: Update the Total row in the tracking table to include all
four columns, adding the missing Notes cell, and change its completed count from
46 to 48 to match the completed values in the rows above.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86a4e781-4224-4297-b1ef-2a99adb8490b
📒 Files selected for processing (2)
TODO.mdconfigs/safety.yaml
| edit_scope: | ||
| enabled: true | ||
| # Paths that the evolution loop is forbidden from modifying | ||
| forbidden_paths: | ||
| - configs/safety.yaml | ||
| - .env | ||
| - Makefile | ||
| # Directories that are entirely off-limits | ||
| forbidden_dirs: | ||
| - .git | ||
| - .github/workflows | ||
| - .evoseal | ||
| - .pytest_cache | ||
| - __pycache__ | ||
| - .venv | ||
| - venv | ||
| # Directories that ARE mutable by the evolution loop | ||
| allowed_dirs: | ||
| - evoseal/ | ||
| - tests/ | ||
| - examples/ | ||
| - docs/ | ||
| - scripts/ | ||
| - benchmarks/ | ||
| - config/ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the edit-scope schema with its consumer.
SafetyIntegration reads edit_scope.allowed_patterns as a mapping, while this file defines allowed_dirs as a list; enabled is also not connected to the top-level enforcement flags used by SafetyIntegration. As written, these settings are inert and the validator falls back to hard-coded defaults instead of using this immutable configuration.
🤖 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 `@configs/safety.yaml` around lines 11 - 35, The edit-scope configuration in
the safety YAML does not match the schema consumed by SafetyIntegration. Update
the edit_scope keys and value types to use allowed_patterns as a mapping, and
connect the enabled setting to the top-level enforcement flags expected by
SafetyIntegration, preserving the existing forbidden paths and directories.
Ensure the resulting immutable configuration is read instead of triggering
validator defaults.
| metric_thresholds: | ||
| # Performance metrics (lower is better) | ||
| duration_sec: | ||
| regression: 0.10 | ||
| critical: 0.25 | ||
| memory_mb: | ||
| regression: 0.10 | ||
| critical: 0.30 | ||
| execution_time: | ||
| regression: 0.10 | ||
| critical: 0.25 | ||
| # Quality metrics (higher is better — negative thresholds) | ||
| success_rate: | ||
| regression: -0.05 | ||
| critical: -0.10 | ||
| pass_rate: | ||
| regression: -0.05 | ||
| critical: -0.10 | ||
| correctness: | ||
| regression: -0.01 | ||
| critical: -0.05 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve thresholds for all monitored metrics.
RegressionDetector uses the supplied metric_thresholds map wholesale rather than merging it with defaults. This omits built-in thresholds for metrics such as accuracy and error_rate, both of which are in the detector’s default monitored set, so they lose their intended per-metric regression and critical limits. Include the complete map or merge configuration with the defaults.
🤖 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 `@configs/safety.yaml` around lines 42 - 62, Expand the metric_thresholds
configuration used by RegressionDetector to include the built-in thresholds for
every default-monitored metric, especially accuracy and error_rate. Preserve the
existing thresholds for duration_sec, memory_mb, execution_time, success_rate,
pass_rate, and correctness, and ensure the supplied map is complete rather than
omitting default entries.
Address review feedback on PR #100: - Add 'configs' to EditScopeValidator._forbidden_dirs so the entire configs/ directory is immutable, not just configs/safety.yaml. This protects any future config files added to the directory. - Mirror the change in safety.yaml's forbidden_dirs for documentation consistency. - Fix TODO.md tracking table: correct total done count (46→48 to match row sums) and add missing Notes cell to Total row. Points from review already handled or not applicable: - .env in sandbox.readonly_files: _apply_readonly_files already skips missing files gracefully (testrunner.py:762). - regression.metric_thresholds critical fallback: RegressionDetector receives the full metric_thresholds dict with both 'regression' and 'critical' keys for every listed metric; the fallback is never reached. - TODO.md commit note about code references: verified all code already uses 'configs/safety.yaml' (plural) consistently.
|
Addressed review feedback. Here's the verdict on each point: ✅ Fixed — missing from : ⏭️ Already handled — ⏭️ Invalid — ⏭️ Minor — TODO.md commit note unverifiable from diff: |
|
Addressed review feedback. Heres my assessment of each point: 1. 2. 3. Redundant 4. TODO.md verification claim — accepted. Updated the resolution note to include specific verification: All checks green: ruff, full pytest (604 passed), detect-secrets. |
Review feedback — verified against codePoint 1:
|
| Point | Verdict | Action |
|---|---|---|
1. config/ budget escape |
Incorrect premise — budget.yaml is not read at runtime |
None |
| 2. Duplicate forbidden dirs | Valid concern, but pre-existing, not a regression from this PR | Follow-up task (optional) |
| 3. Default-deny for ambiguous paths | Already handled correctly | None |
No code changes needed for this PR.
Multiple safety-critical modules (edit_scope_validator, testrunner, doctor, safety_integration) reference configs/safety.yaml as the immutable safety configuration, but the file did not exist. Only config/ (singular) existed with budget.yaml, logging.yaml, etc. Created configs/safety.yaml with defaults for: - Edit-scope enforcement (forbidden paths/dirs, allowed mutable surface) - Regression detection thresholds (per-metric regression/critical levels) - Checkpoint/rollback policy (max checkpoints, auto-rollback on critical) - Sandboxed test execution (readonly files, resource limits) This resolves evoseal doctor reporting 'safety.yaml not found' and ensures safety tests assert against a real file on disk.
Address review feedback on PR #100: - Add 'configs' to EditScopeValidator._forbidden_dirs so the entire configs/ directory is immutable, not just configs/safety.yaml. This protects any future config files added to the directory. - Mirror the change in safety.yaml's forbidden_dirs for documentation consistency. - Fix TODO.md tracking table: correct total done count (46→48 to match row sums) and add missing Notes cell to Total row. Points from review already handled or not applicable: - .env in sandbox.readonly_files: _apply_readonly_files already skips missing files gracefully (testrunner.py:762). - regression.metric_thresholds critical fallback: RegressionDetector receives the full metric_thresholds dict with both 'regression' and 'critical' keys for every listed metric; the fallback is never reached. - TODO.md commit note about code references: verified all code already uses 'configs/safety.yaml' (plural) consistently.
- Add comment explaining intentional configs/ vs config/ directory split - Remove redundant configs/safety.yaml from forbidden_paths (already covered by forbidden_dirs containing 'configs') - Add verification evidence to TODO.md resolution note
cec4ed7 to
d27df11
Compare
What
Created
configs/safety.yaml— the immutable safety configuration file referenced by multiple safety-critical modules but never actually created.Why
Multiple modules reference
configs/safety.yaml(pluralconfigs/) as the immutable safety configuration:evoseal/core/edit_scope_validator.py— lists it as a forbidden/immutable pathevoseal/core/testrunner.py— mounts it read-only during sandboxed test executionevoseal/cli/commands/doctor.py— validates its well-formednessevoseal/core/safety_integration.py— references it in docs/commentsOnly
config/(singular) existed, containingbudget.yaml,logging.yaml, etc. —configs/safety.yamlwas missing entirely. This causedevoseal doctorto report 'safety.yaml not found' and meant safety tests were asserting against a nonexistent file.What's in the file
Sensible defaults for:
RegressionDetectordefaults)What was NOT changed
No Python code was modified. The existing code already references
configs/safety.yaml(plural) consistently — the only gap was the missing file itself.Addresses
TODO.md: "Fix missing
configs/safety.yamlandconfig/vsconfigs/path discrepancy" (P1)Verification
ruff format --check .— 392 files already formattedruff check evoseal/ tests/— All checks passedpytest— 604 passed, 23 deselectedyaml.safe_load()Summary by CodeRabbit
configs/directory.