chore: bump 0.37.0 + hermetic adaptive_scaling test fix + rust-gym scorecard baseline (#4521) - #4533
Conversation
rysweet
left a comment
There was a problem hiding this comment.
Comprehensive Code Review — Step 17b
Scope reviewed: Cargo.toml/Cargo.lock (0.36.0 → 0.37.0), new ooda/scorecard.json (rust-expert calibration baseline), and the hermetic test fix in tests/adaptive_scaling.rs.
Verdict: Approve with minor nits — no blocking issues.
Checklist
- ✅ Code quality & standards — clean; no logic changes.
- ✅ Test coverage — the test change improves correctness (removes env-dependent flakiness).
- ✅ No TODOs, stubs, or swallowed exceptions.
- ✅ No unimplemented functions.
- ✅ Logic correctness — verified (see below).
- ✅ Edge-case handling — the fix specifically addresses the
SIMARD_SCALING=autohost edge case.
Strengths
tests/adaptive_scaling.rsis the substantive change and it is correct. Settingscaler: Nonebefore..OodaConfig::default()makesscaler_current_max_can_override_confighermetic:OodaConfig::default()readsSIMARD_SCALINGfrom the process env (src/ooda_loop/types.rs:368), so on a host withSIMARD_SCALING=autothe inherited default scaler (ceiling 24) would callscaler.adjust()and override themax_concurrent_actionscap under test. The explicitscaler: Nonematches the already-established pattern insrc/ooda_loop/decide.rs:180,217,591and its rationale comment (decide.rs:208-217). Well-documented via issue #2732.- Version bump is routine;
Cargo.lockkept in sync.
Non-blocking nits (see inline)
ooda/scorecard.jsonis a generated artifact (produced bysrc/bin/simard_rust_gym.rs, whose default output istarget/simard-rust-gym/). Checking in a snapshot is reasonable as a #1241 calibration baseline, but risks staleness/churn; consider documenting the regeneration command near it.ooda/scorecard.jsonhas no trailing newline (serde_json::to_vec_prettyomits it). If a pre-commit end-of-file-fixer runs it will rewrite the file — notepre-commitis currentlypendingin CI. Consider emitting a trailing\nfrom the gym writer.- PR title mismatch: "Update documentation with 12 changed files (#4521)" does not describe this 4-file change (version bump + scorecard + test fix). Recommend retitling for accurate history.
None of these block merge.
| // `scaler: None` keeps the test hermetic (issue #2732). | ||
| let config = OodaConfig { | ||
| max_concurrent_actions: scaler.current_max(), | ||
| scaler: None, |
There was a problem hiding this comment.
✅ Correct and consistent with the established pattern in src/ooda_loop/decide.rs:180,217,591. Explicit scaler: None overrides the env-derived default from OodaConfig::default() (types.rs:368 reads SIMARD_SCALING), keeping this test hermetic on SIMARD_SCALING=auto hosts. Good fix + clear rationale comment referencing #2732.
| "min_gap": 0.4, | ||
| "passed": true | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Nit: no trailing newline (serde_json::to_vec_pretty in src/bin/simard_rust_gym.rs doesn't append one). A pre-commit end-of-file-fixer would rewrite this file — pre-commit is currently pending in CI. Also: this is a generated artifact (gym default output is target/simard-rust-gym/); if it is intended as a checked-in calibration baseline, consider a short note documenting how/when to regenerate it.
🔒 Security Review — Step 17c (MANDATORY)Scope: PR #4533 — 4 files, +363/-3 ( Verdict: ✅ PASS — No high-confidence security issues found. No blocking items. Checklist
Supply chain
No remediation required. Security review cleared for merge. |
Philosophy Compliance Review — Step 17dScope: PR #4533 (4 files, +363/-3) — version bump 0.36.0→0.37.0, new checked-in Compliance Checklist
AssessmentPASS — zero required changes. The core change makes the test hermetic: setting Non-blocking observations (philosophy-adjacent, out of scope)
Verdict: Philosophy-compliant. No implementation required. |
Step 18b — Review Feedback Implementation (MANDATORY)Implemented the non-blocking suggestions from Steps 17b/17c/17d + 18a. No blocking issues existed, so no code re-work was required. Summary per feedback item:
Disagreement documented
No new issues introducedChanges were metadata-only (PR title + description). No source, test, or dependency files were modified, so there is no risk of regressions. All non- Verdict: All actionable feedback addressed. PR #4533 is ready for merge review. |
Summary
Version bump
0.36.0 → 0.37.0plus a hermetic test fix and a new rust-competency-gym scorecard baseline. (Original workflow-generated title/body were inaccurate — corrected in Step 18b review feedback.)Issue
Closes #4521
Changed files (4)
Cargo.toml— version0.36.0 → 0.37.0Cargo.lock— mirror of thesimardpackage version bump (no dependency changes)tests/adaptive_scaling.rs— addscaler: Nonebefore..OodaConfig::default()so the test no longer inherits aSIMARD_SCALING-derived default scaler that could override the explicitmax_concurrent_actionscap under test (hermetic test fix)ooda/scorecard.json— new checked-in rust-gym calibration baseline (+353)Diff stat
Note on
ooda/scorecard.json(Step 18b review feedback)ooda/scorecard.jsonis a generated artifact, not a hand-authored file. It is produced by thesimard-rust-gymbinary:The file has no trailing newline by design: it is serialized via
serde_json::to_vec_pretty, which does not emit one. This is intentional and left as-is so the checked-in snapshot matches generator output exactly (adding a manual newline would create spurious diffs on every regeneration). The repo's pre-commit gate is Python-free (issue #3181) and runs onlycargo fmt/clippy/rust-only gate — it has no end-of-file fixer, so the missing newline does not affect CI.