Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions experiments/PR-239-REVIEW-DISPOSITIONS.md
Original file line number Diff line number Diff line change
@@ -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.