diff --git a/experiments/PR-239-REVIEW-DISPOSITIONS.md b/experiments/PR-239-REVIEW-DISPOSITIONS.md new file mode 100644 index 000000000..b2cf1a5cc --- /dev/null +++ b/experiments/PR-239-REVIEW-DISPOSITIONS.md @@ -0,0 +1,25 @@ +# PR #239 review-findings dispositions + +The code review of the experiments work on [PR #239](https://github.com/SamGu-NRX/BaseScanning/pull/239) left 15 unresolved findings (1 marked P1). Every one was already fixed on `alpha` by an earlier merged commit before the follow-up review pass; this record ties each finding to the fix commit and the regression test that covers it, verified by reading the current source at `alpha` head `969d4e93` on 2026-10-17 and by running the suites. + +Suites at that head: meter-closeup 401 passed, evals 259, evals models 46, scoring 372, capture-contract 19, live-dots fixtures 20 — 1,117 passed, 0 failed, ruff check and ruff format clean. The `experiments/evals` project pins `tool.uv.required-version >=0.11.15, <0.12`; the `experiments/evals/models` project has no uv version pin. The evals run used uv 0.11.33. No mypy or pyright target exists for these directories; ruff is the static gate the repository runs here. + +| Finding | Disposition | Fixed in | Covered by | +| --- | --- | --- | --- | +| [P1: rank candidates without consulting the label](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4274619166) | already-fixed-on-alpha | `43698f23` | `tests/test_serial_contract.py::test_field_cli_reports_the_blind_second_rank_and_uses_the_blind_box`; `evaluate()` orders with `ranked(found)` and compares the target only after ordering | +| [Preserve output permissions during atomic replacement](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4273527775) | already-fixed-on-alpha | `7b5529dd` | `test_contract.py::test_atomic_replacement_preserves_existing_permissions`, `::test_new_result_permissions_follow_umask` | +| [Reject cleanup paths nested inside directory sessions](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4273706605) | already-fixed-on-alpha | `d8573f02` | `tests/test_field.py::test_score_preserves_directory_sessions_overlapping_outputs` | +| [Remove or reject a symlinked scored directory](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4275677307) | already-fixed-on-alpha | `4e098eb5` | `tests/test_field.py::test_score_replaces_scored_symlink_without_touching_its_target` (live and dangling) | +| [Remove the blanket commercial-license claim](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4274029614) | already-fixed-on-alpha | `01ec183f` | Documentation change, no test; `models/README.md` excludes MoGe-2 from commercial-use claims and marks the weights unverified | +| [Reclaim fixture stages after every hard exit](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4273527766) | already-fixed-on-alpha | `7b5529dd` | `test_fetch_fixtures.py::test_failed_refresh_preserves_existing_fixtures`, `::test_overlapping_refresh_waits_before_recovery` | +| [Verify stage ownership before recovery cleanup](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4274029597) | already-fixed-on-alpha | `01ec183f` | `test_fetch_fixtures.py::test_recovery_preserves_unknown_fixture_stages`, `::test_recovery_preserves_a_stage_without_a_marker` | +| [Preserve backups when recovery finds an occupied destination](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4273706594) | already-fixed-on-alpha | `d8573f02` | `test_fetch_fixtures.py::test_failed_refresh_preserves_existing_fixtures` | +| [Describe the field ranking as blind](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4275677292) | already-fixed-on-alpha | `4e098eb5` | `tests/test_serial_contract.py::test_field_help_describes_blind_ranking` | +| [Ignore unsupported Unicode in barcode payloads](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4275092098) | already-fixed-on-alpha | `a2480273` | `tests/test_serial_contract.py::test_invalid_barcodes_do_not_abort_photo_scoring` (é, combining accent, 序號, ß, ff payloads) | +| [Split barcode payloads before validating serial characters](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4275363449) | already-fixed-on-alpha | `a7e99b4a` | `tests/test_serial_contract.py::test_barcode_fields_are_validated_independently`, `::test_barcode_fields_cannot_join_across_a_delimiter` | +| [Compute `alone` per OCR row](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4274619180) | already-fixed-on-alpha | `d8d31063` | `tests/test_serial_contract.py::test_two_number_rows_are_each_alone_and_keep_the_prior_rule_pick`, `::test_row_features_use_the_source_row_and_keep_candidate_box_separate` | +| [Reject non-ASCII serials before compatibility expansion](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4275092116) | already-fixed-on-alpha | `a2480273` | `tests/test_serial_contract.py::test_unicode_expansions_are_invalid_as_targets_and_ocr`, `::test_only_fullwidth_ascii_is_mapped` | +| [Exclude text-only serial candidates](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4274619193) | already-fixed-on-alpha | `d8d31063` | `tests/test_serial_contract.py::test_maker_only_observation_requires_a_number_retake`, `::test_maker_on_another_row_cannot_be_the_number_candidate` | +| [Update the documented endpoint for capture timing](https://github.com/SamGu-NRX/BaseScanning/pull/239#discussion_r4274029602) | already-fixed-on-alpha | `01ec183f` | `tests/test_review_debt.py::test_capture_duration_includes_last_refusal`, `::test_refusal_only_session_has_capture_duration`, `::test_refusal_before_start_is_rejected`; `FORMATS.md` matches the max-over-measurements-and-refusals rule | + +The iOS (`measure-lab` Swift package) tests need Xcode and were not run in the Linux verification environment; the Python fixture-recovery suite that covers the live-dots findings ran in full.