From 3be42cd6212eb181ef4f40948be0230129052ce0 Mon Sep 17 00:00:00 2001 From: Obvious Date: Sat, 17 Oct 2026 23:44:11 +0000 Subject: [PATCH 1/2] docs(experiments): record the PR #239 review-findings dispositions All 15 unresolved review findings on PR #239 were already fixed on alpha by earlier merged commits. This file ties each finding to its fix commit and the regression test that covers it, and records the verification run (1,117 passed, 0 failed across the six suites at alpha 969d4e93). Co-authored-by: Sam Gu --- experiments/PR-239-REVIEW-DISPOSITIONS.md | 25 +++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 experiments/PR-239-REVIEW-DISPOSITIONS.md diff --git a/experiments/PR-239-REVIEW-DISPOSITIONS.md b/experiments/PR-239-REVIEW-DISPOSITIONS.md new file mode 100644 index 000000000..e25ae2289 --- /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 suite projects pin `tool.uv.required-version >=0.11.15, <0.12`, so the runs 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. From 47034ce2d0f84f775cb63b28135c407f9d0a907c Mon Sep 17 00:00:00 2001 From: SamGu-NRX Date: Thu, 22 Oct 2026 09:10:12 -0500 Subject: [PATCH 2/2] docs: scope PR239 uv version wording --- experiments/PR-239-REVIEW-DISPOSITIONS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experiments/PR-239-REVIEW-DISPOSITIONS.md b/experiments/PR-239-REVIEW-DISPOSITIONS.md index e25ae2289..b2cf1a5cc 100644 --- a/experiments/PR-239-REVIEW-DISPOSITIONS.md +++ b/experiments/PR-239-REVIEW-DISPOSITIONS.md @@ -2,7 +2,7 @@ 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 suite projects pin `tool.uv.required-version >=0.11.15, <0.12`, so the runs used uv 0.11.33. No mypy or pyright target exists for these directories; ruff is the static gate the repository runs here. +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 | | --- | --- | --- | --- |