Merged
Conversation
…rol and read it against the FP arm P3-15'-f measured criterion (b)'s seven ADR-0006 A4 rule parameters on 278 de-novo consensuses of round-0 FP-manifest candidates — a population of unknown status — and the user declined to pin the parameters on that plus an n=1 positive control. P3-15'-g/-g-ii/-g-iii built the matched control the choice needed: Stage-1 re-detected spans on held-out curated T-box records, one per ADR-0004 cluster, order-stratified, searched and aligned by the same instrument (SLURM job 1264, 76 consensuses of 317 queries over 160 records). This step measures it at P3-15'-f's identical grid and reads the two arms against each other. Inputs - two.amlab:$HOME/tbox-scratch/round_p3_15g_control/msa (76 msa.sto, job 1264) - data/processed/mining/curated_control_manifest_v0.json (317 queries / 160 records) - reports/p3/architecture_parameter_measurement.json (the P3-15'-f FP arm) Outputs - src/tbox_finder/mining/architecture_param_control_compare.py - reports/p3/architecture_parameter_measurement_control.json - reports/p3/architecture_parameter_control_comparison.json - data/processed/mining/curated_control_status_v0.json (merged 317-row status table) - tests/unit/test_architecture_param_control_compare.py (43 tests) - 11 seam tests in tests/unit/test_architecture_param_measure.py - analyses/phase3_log.qmd stanza The seam, and why it changes nothing measurable architecture_param_measure gains a frozen SupplyArm registry (--arm) supplying only the four strings a report makes about ITSELF: step, disclosure, criterion (a)'s control_note, and the provenance rule. Shipping the FP arm's prose over the control's numbers would be a committed public report describing the wrong corpus, and no internal check could see it because every count would still reconcile. The sweep and every count are arm-independent — that identity is what makes the control matched — and --arm round0_fp is the default, so the committed FP report re-derives BYTE-IDENTICALLY across the seam (verified against the retrieved supply; only generated_at_utc, git_sha and env_lock_hash differ). candidate_state is extracted from evaluate_tuple's loop body so the record-level aggregation reads the same code the counts come from. Result (b)'s share of `failed` among decided candidates, control vs FP, at the identical grid: loosest_nonvacuous 8/76=0.105 25/278=0.090 diff -0.015 sensitive_core 11/76=0.145 52/278=0.187 diff +0.042 canonical_core_3helix 39/76=0.513 141/278=0.507 diff -0.006 canonical_core_3helix_ncca3 49/76=0.645 220/278=0.791 diff +0.147 curated_freeze_transplant 72/76=0.947 265/278=0.953 diff +0.006 strictest 75/76=0.987 271/278=0.975 diff -0.012 The largest gap on the named grid is 14.7 pp; at three of six settings the control fails MORE than the FP arm; at 5 of 6 the control's record-level 95% Wilson interval (n = 68 producible records) contains the FP arm's point estimate. (b) is behaving as a severity knob on both populations rather than as a discriminator between them. Every interval is on the record-level n. 149 of 160 records contribute 2 queries and 4 contribute 3, so control_query_level.ci95 is null in every row with the reason beside it; the CI is n = 68 throughout. Also measured: the instrument is producible for 76/317 = 24.0 % of control queries against 278/941 = 29.5 % of FP candidates (68/160 = 42.5 % of records); the self-hit floor caveat, re-derived from the control's OWN rows, is 5 of 317 queries = 1.58 pp and does not close that gap. Alignment matchedness survived the query matchedness (depth median 30 vs 34, width median 99 vs 106). Criterion (a) itself passes only 30/76 = 39.5 % of the control against 167/278 = 60.1 % of the FP arm, so (a)-passed cannot be read as an enrichment for T-box identity. pins_nothing: true in both reports. The ADR-0006 A4 seven-parameter choice remains the open §7 decision; this step informs it and does not take it. Validation - tests/unit + tests/golden vs a matched main baseline at c1a5b97: ZERO new failures (worktree 13 FAILED, main 26, a strict subset; the whole delta is 14 tests moving to SKIPPED on unmaterialised DVC/LFS inputs, and passes rise by exactly the 54 tests added). Both ^FAILED sets ANSI-stripped and cross-checked against the summary line. - 44 sabotages, all RED against their NAMED test, + 2 negative controls green. Two initially stayed green and both were fixture weaknesses, fixed by rebuilding the fixtures rather than the sabotages. - Both new reports re-derive byte-identically apart from generated_at_utc. - ruff + black clean. No SLURM job. No ADR amendment. No DVC object.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds arm-aware architecture parameter measurement and a matched-control comparison CLI. It validates supplies and reports, recomputes control states, calculates record-level statistics, and commits measurement and comparison JSON reports with provenance. ChangesArchitecture parameter analysis
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant compare
participant MeasurementReport
participant ControlSupply
participant ComparisonReport
CLI->>compare: invoke compare command
compare->>MeasurementReport: validate grids and tuple labels
compare->>ControlSupply: load manifests, statuses, and consensus records
compare->>compare: reconstruct control states and calculate statistics
compare->>ComparisonReport: write provenance-rich JSON output
ComparisonReport-->>CLI: report summary and exit status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/unit/test_architecture_param_control_compare.py (1)
688-728: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
walkhelper so the positive control exercises the real scan.Lines 697-705 and 717-725 define the same nine-line
walkfunction twice.test_the_absolute_path_scan_can_actually_failis the positive control for the scan, but it validates a copy. If one copy is edited and the other is not, the positive control no longer proves anything about the scan that runs against the payload.♻️ Proposed extraction
+def absolute_path_leaks(payload) -> list[str]: + """Every string in ``payload`` that starts with '/', with its JSON path.""" + leaks: list[str] = [] + + def walk(node, path=""): + if isinstance(node, dict): + for k, v in node.items(): + walk(v, f"{path}/{k}") + elif isinstance(node, list): + for i, v in enumerate(node): + walk(v, f"{path}[{i}]") + elif isinstance(node, str) and node.startswith("/"): + leaks.append(f"{path}: {node}") + + walk(payload) + return leaks + + def test_main_publishes_no_absolute_path_anywhere_in_the_payload(control_files, reports, tmp_path): """The P3-15'-f lesson: a home directory and an account name leaked into a PUBLIC report. The whole payload is walked, not the fields I remembered.""" out = tmp_path / "comparison.json" assert cmp.main(cli_args(control_files, reports, out)) == 0 - payload = json.loads(out.read_text()) - - leaks: list[str] = [] - - def walk(node, path=""): - ... - walk(payload) - assert leaks == [] + assert absolute_path_leaks(json.loads(out.read_text())) == [] def test_the_absolute_path_scan_can_actually_fail(control_files, reports, tmp_path): """Positive control for the scan above — on P3-15'-f the equivalent control could not fail, because its subject never started with '/'.""" - payload = {"a": {"b": ["/home/someone/x"]}} - ... - assert leaks == ["/a/b[0]: /home/someone/x"] + assert absolute_path_leaks({"a": {"b": ["/home/someone/x"]}}) == ["/a/b[0]: /home/someone/x"]🤖 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 `@tests/unit/test_architecture_param_control_compare.py` around lines 688 - 728, Extract the duplicated nested walk helper from test_main_publishes_no_absolute_path_anywhere_in_the_payload into a shared test-level helper, then reuse it in both tests, including test_the_absolute_path_scan_can_actually_fail. Preserve the existing recursive traversal, path formatting, and leak collection behavior so the positive control exercises the same scan implementation as the main test.src/tbox_finder/mining/architecture_param_control_compare.py (2)
67-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImporting
_sha256_ofreaches into another module's private surface.
_sha256_ofcarries a leading underscore inarchitecture_param_measure. Two modules now depend on it. Promote it to a public name in its home module, or move it to the shared provenance helper, so a future rename inarchitecture_param_measurecannot break this module silently.🤖 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 `@src/tbox_finder/mining/architecture_param_control_compare.py` around lines 67 - 76, Remove the private `_sha256_of` dependency from the import list in the comparison module by promoting it to a public symbol in `architecture_param_measure` or relocating it to the shared provenance helper, then update all references and exports to use the public name consistently.
728-756: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHardcoded population counts in prose will go stale when the draw is re-run.
Line 752 states "241 of 317 queries, 92 of 160 records" as a literal string. These counts are correct for this run: 317 − 76 = 241 and 160 − 68 = 92. They are also already computed in
producibilityand passed throughcompare. If the control draw is re-run, this sentence keeps the old numbers and nothing in the report shows the drift.
load_detectstates the opposing principle at lines 715-718: a number retyped here goes stale without the report showing it. Apply the same rule. The same concern applies to line 442 ("149 of the 160 control records contribute 2 queries and 4 contribute 3") and line 466 ("the 941 FP candidates were carved from 76 assemblies").♻️ Proposed change: pass the counts in
def limitations( - detect_report_path: str | Path | None, detect: Mapping[str, Any] | None + detect_report_path: str | Path | None, + detect: Mapping[str, Any] | None, + *, + prod: Mapping[str, Any], ) -> dict[str, Any]: """Everything that bounds how far this rate may be carried.""" + q = prod["control_query_level"] + rec = prod["control_record_level"] + n_q_unavailable = q["n_queries"] - q["n_producible"] + n_rec_unproducible = rec["n_records"] - rec["n_records_with_a_producible_query"] out: dict[str, Any] = {"the_control_measures_the_instrument_not_the_biology": ( - "a control record that resolves no consensus (241 of 317 queries, 92 of 160 " - "records) is spared under ADR-0005 D14 and leaves the denominator; the rates " - "are conditional on the instrument having produced an alignment at all." + f"a control record that resolves no consensus ({n_q_unavailable} of " + f"{q['n_queries']} queries, {n_rec_unproducible} of {rec['n_records']} " + "records) is spared under ADR-0005 D14 and leaves the denominator; the rates " + "are conditional on the instrument having produced an alignment at all." ),Then update the call site at line 708 to
limitations(detect_report_path, detect, prod=prod).🤖 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 `@src/tbox_finder/mining/architecture_param_control_compare.py` around lines 728 - 756, Update limitations to accept the producibility data via a prod parameter, and update its call site to pass prod. Replace the hardcoded counts in the control, query, and FP-arm explanatory strings—including the “241 of 317,” “92 of 160,” “149 of the 160,” and “941 … 76 assemblies” statements—with values derived from prod so reruns report current counts.src/tbox_finder/mining/architecture_param_measure.py (2)
1181-1191: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBind
--armto its paths, or refuse the mismatched combination.
--manifestdefaults to the FP manifest (line 1160) and--outdefaults to the FP report path (line 1194). Neither default follows--arm. If an operator runs--arm curated_controland forgets--out, the run overwritesreports/p3/architecture_parameter_measurement.jsonwith control counts and control prose. The result is internally consistent, so no check in this module detects it.Refuse the combination after parsing. Keep the FP defaults so the committed FP report still re-derives byte-identically.
🛡️ Proposed guard in
maintry: supply_arm = resolve_arm(args.arm) + # ⚠ `--manifest` and `--out` default to the FP arm's paths and do NOT track + # `--arm`. A non-default arm written to the FP report's path would publish + # control counts under the FP filename, and every count would still reconcile. + if supply_arm.key != DEFAULT_ARM: + fp_defaults = build_parser().parse_args(["measure", "--msa-root", args.msa_root]) + left_at_fp_default = [ + name + for name, got, default in ( + ("--manifest", args.manifest, fp_defaults.manifest), + ("--out", args.out, fp_defaults.out), + ) + if got == default + ] + if left_at_fp_default: + raise MeasureError( + f"--arm {supply_arm.key!r} was selected but {left_at_fp_default} " + "still carry the round0_fp defaults; pass them explicitly" + ) body = measure(🤖 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 `@src/tbox_finder/mining/architecture_param_measure.py` around lines 1181 - 1191, Update main to validate the parsed --arm, --manifest, and --out combination after argument parsing. Preserve the existing FP defaults and allow the default FP arm unchanged, but refuse curated_control when either path still points to the FP default, requiring explicit control paths instead of overwriting the FP report.
87-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCentralize comparison ground-truth metadata.
SupplyArm.ground_truthhas no production reader.architecture_param_control_compare.pyemits equivalent values as independent literals. Derive those values from the arm registry, or removeground_truthand its test if the comparison module must remain report-driven.The “four self-descriptions” wording is correct for the arm-specific measurement fields.
positive_control.reasonis separate, andground_truthis not emitted bymeasure().🤖 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 `@src/tbox_finder/mining/architecture_param_measure.py` around lines 87 - 114, Centralize ground-truth metadata by updating architecture_param_control_compare.py to derive comparison values from the SupplyArm registry rather than duplicating literals, ensuring arm-specific fields remain distinct from positive_control.reason. Alternatively, remove the unused SupplyArm.ground_truth field and its associated test if the comparison remains report-driven; do not treat ground_truth as measure() output.
🤖 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 `@src/tbox_finder/mining/architecture_param_control_compare.py`:
- Around line 120-153: Validate every member of the operator-supplied joint list
at src/tbox_finder/mining/architecture_param_control_compare.py:120-153 is a
Mapping before calling c_row.get or f_row.get, raising CompareError for
malformed rows. Likewise validate every rows member at
src/tbox_finder/mining/architecture_param_control_compare.py:228-239 before
accessing candidate_id, and replace any candidate_id-based comprehension that
absorbs duplicates with explicit duplicate detection so repeated IDs are refused
through the existing exit-3 error path.
---
Nitpick comments:
In `@src/tbox_finder/mining/architecture_param_control_compare.py`:
- Around line 67-76: Remove the private `_sha256_of` dependency from the import
list in the comparison module by promoting it to a public symbol in
`architecture_param_measure` or relocating it to the shared provenance helper,
then update all references and exports to use the public name consistently.
- Around line 728-756: Update limitations to accept the producibility data via a
prod parameter, and update its call site to pass prod. Replace the hardcoded
counts in the control, query, and FP-arm explanatory strings—including the “241
of 317,” “92 of 160,” “149 of the 160,” and “941 … 76 assemblies”
statements—with values derived from prod so reruns report current counts.
In `@src/tbox_finder/mining/architecture_param_measure.py`:
- Around line 1181-1191: Update main to validate the parsed --arm, --manifest,
and --out combination after argument parsing. Preserve the existing FP defaults
and allow the default FP arm unchanged, but refuse curated_control when either
path still points to the FP default, requiring explicit control paths instead of
overwriting the FP report.
- Around line 87-114: Centralize ground-truth metadata by updating
architecture_param_control_compare.py to derive comparison values from the
SupplyArm registry rather than duplicating literals, ensuring arm-specific
fields remain distinct from positive_control.reason. Alternatively, remove the
unused SupplyArm.ground_truth field and its associated test if the comparison
remains report-driven; do not treat ground_truth as measure() output.
In `@tests/unit/test_architecture_param_control_compare.py`:
- Around line 688-728: Extract the duplicated nested walk helper from
test_main_publishes_no_absolute_path_anywhere_in_the_payload into a shared
test-level helper, then reuse it in both tests, including
test_the_absolute_path_scan_can_actually_fail. Preserve the existing recursive
traversal, path formatting, and leak collection behavior so the positive control
exercises the same scan implementation as the main test.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23299e12-77d8-47cb-a5fc-84a708bed938
⛔ Files ignored due to path filters (2)
analyses/phase3_log.qmdis excluded by!**/*.qmddata/processed/mining/curated_control_status_v0.jsonis excluded by!data/**
📒 Files selected for processing (6)
reports/p3/architecture_parameter_control_comparison.jsonreports/p3/architecture_parameter_measurement_control.jsonsrc/tbox_finder/mining/architecture_param_control_compare.pysrc/tbox_finder/mining/architecture_param_measure.pytests/unit/test_architecture_param_control_compare.pytests/unit/test_architecture_param_measure.py
…guard two escapes
CodeRabbit CLI round 1: 3 findings, all real (1 major, 2 minor).
major — three emitted strings stated corpus counts as LITERALS ("149 of the 160
control records contribute 2 queries and 4 contribute 3"; "241 of 317 queries, 92 of
160 records"; "the 941 FP candidates were carved from 76 assemblies"). They are true
only of the job-1264 supply and were already false on this module's own 4-record test
fixture — a report stating a corpus it did not measure, which is the same defect as a
report naming the wrong arm and is equally invisible from inside because every count
still reconciles. The first two are now formatted from the values the report already
computes (compare_tuple derives them from `by_record`; `limitations` takes `prod`).
The third cannot be recomputed here — the 76-assemblies figure belongs to P3-15'-g —
so it is stated qualitatively with a pointer rather than as a literal that will go
stale. The module docstring keeps the concrete sizes but now says they describe the
supply it was written for.
minor — `producibility` divided by the FP report's `n_candidates_in_manifest`, so a
report carrying 0 raised ZeroDivisionError, which `main` does not catch: the CLI
would exit 1 with a traceback while every other bad input exits 3. Refused instead of
defaulted — an FP report with no candidates is not a comparator.
minor — `load_status` checked that `rows` was a non-empty list but not the shape of
each row, so a bare string or a row missing `n_homologs`/`msa_depth` raised
TypeError/KeyError out of the comprehension and out of `self_hit_floor_caveat`. Same
escape, same convention: refused with exit 3.
7 new tests (50 in the file), each RED under a named sabotage; the harness is now 50
sabotages + 2 negative controls, 0 problems. The comparison report is regenerated —
every headline number is unchanged, only the derived prose moved.
…g duplicate, and a footgun that clobbered a committed report CodeRabbit GitHub app on 3a97aca: 1 actionable + 5 nitpicks. All 6 real, all taken. The app found two things the CLI round missed, and one of them is the sibling of the guard the CLI round made me add. 1 (actionable) — `assert_grids_identical` called `c_row.get("label")` on `joint` members without checking they are Mappings. Round 1 added exactly this guard to `load_status`'s rows and left its sibling untouched ([[fixed-one-of-two-identical-things]]). Both arms are now checked, and the refusal names which arm carried the defect. 2 — `load_status` built `row_status` as a dict comprehension, so two rows sharing a `candidate_id` merged with last-write-wins: a query's real status could be overwritten while the map/rows agreement check still passed and every downstream count still reconciled ([[duplicate-key-merges-instead-of-colliding]]). Built as a list first and refused. 3 — `SupplyArm.ground_truth` had no production reader while the comparison emitted its own independent spelling of the same fact ([[pinned-constant-that-nothing-reads]]). `arm_for_step` now recovers each report's arm from the `step` it declares, and both `ground_truth` values are read off the registry. That turns a dead constant into a binding: a report whose step no arm declares is refused, and two reports of the same ground_truth are refused as "one arm read against itself". 4 — `--out` and `--manifest` default to the FP arm's paths and neither follows `--arm`, so `--arm curated_control` with a forgotten `--out` would overwrite the committed P3-15'-f report with control counts AND control prose — internally consistent, undetectable downstream. The defaults stay (that is what keeps the FP report byte-identically re-derivable); the mismatched COMBINATION is refused. ⚠ THIS ONE BIT DURING THE STEP. The sabotage proving the new guard bites removed the refusal, and the guard's own test — which passed `apm.DEFAULT_OUT` from the repo-root cwd — then wrote a 4-consensus fixture over reports/p3/architecture_parameter_measurement.json. Caught by re-deriving the FP report and diffing, not by any test. Restored from HEAD and independently re-verified byte-identical against the real 278-consensus supply. The test now `monkeypatch.chdir`s, and the sabotage harness gained a backstop: `git status --porcelain` before and after every sabotage, so a run that mutates the tree raises instead of passing. 5 — `_sha256_of` was imported across module boundaries by two modules. Promoted to `sha256_of` with the private name kept as an alias; three call sites updated. 6 — the absolute-path scan's `walk` helper was defined twice, so its positive control validated a copy. Extracted to `absolute_path_leaks`; both now run the same function. 18 new tests (128 across the two files), each RED under a named sabotage; the harness is now 57 sabotages + 2 negative controls, 0 problems. Both new reports regenerated — every headline number unchanged. tests/unit + tests/golden vs the matched main baseline at c1a5b97: still ZERO new failures (13 vs 26, a strict subset). ruff + black clean.
…n the tuple, normalize the path CodeRabbit CLI round 3 on 26eb395: 3 findings, all real, all minor, all taken. 1 — `load_status`'s round-2 guard checked that the four keys EXIST but not that `n_homologs`/`msa_depth` are integers. `self_hit_floor_caveat` calls `int()` on both, and `int(None)` raises TypeError: the same escape the key check closes, one level down. `bool` is excluded explicitly, since it is an `int` subclass and `True` would otherwise pass as a homolog count. 2 — `main`'s refusal tuple omitted TypeError, so the convention it documents ("every operator-supplied path refuses with exit 3") did not hold for all malformed input. Widened. ⚠ My first test for this was VACUOUS and the sabotage caught it: it fed a null depth field, which finding 1's new guard now refuses with CompareError before any `int()` runs — so it exercised the guard, not the tuple, and removing TypeError from the tuple left it green. Rewritten against a genuinely unguarded path: a null `n_consensuses_measured` in the FP report, which reaches `int()` directly. 3 — the `--arm`/`--out` guard compared `str(args.out) == DEFAULT_OUT`, so `./reports/p3/...` or `reports/p3/../p3/...` walked straight through and wrote the control's numbers over the committed report — the exact outcome the guard exists to prevent. Compared on resolved paths now, with the residual gap stated in the comment rather than papered over (DEFAULT_OUT resolves against the current cwd, so an absolute path aimed at a checkout the process is not standing in is still not caught). 6 new tests (140 across the two files), each RED under a named sabotage; the harness is now 61 sabotages + 2 negative controls, 0 problems, and it re-checks `git status --porcelain` around every sabotage. Both reports regenerated, headline unchanged. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean.
… must spell None the way production does CodeRabbit CLI round 4 on c8b10e6: 1 finding, real, minor. `test_the_committed_comparison_is_internally_consistent` recomputed the headline's "control fails more than FP" count with a bare `< 0`, while production spells it `(… or 0) < 0`. `fp_minus_control_query_share_failed` is None whenever an arm decided nothing, and `None < 0` raises TypeError rather than evaluating False — so on a future supply where any setting decides nothing the artifact test would error out instead of checking the artifact. Mirrored. Added the edge case as a real unit test: an FP row with zero decided candidates must give `share_failed: None`, `fp_minus_control…: None` and `control_record_ci_contains_fp_point: None` rather than a number or a crash, and the headline's own expression must survive it. RED under a named sabotage (63rd). Harness: 62 sabotages + 2 negative controls, 0 problems. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean.
… stratified contrast Not a review finding: a confound I went looking for while the CodeRabbit rate limit reset. (a) and (b) read the same alignment (ADR-0006 A4) and the two arms do not have the same (a) composition — (a) passes 30/76 of the control against 167/278 of the FP arm — so the unstratified similarity between them could have been a composition artifact, or could have hidden arms that differ inside every stratum while agreeing overall. `stratify_by_criterion_a` now reports (b)'s failure share within each (a) stratum for both arms, per named setting. It is not a reversal: at `sensitive_core` the control fails 3/30 = 0.100 of its (a)-passed against the FP arm's 20/167 = 0.120, and 8/46 = 0.174 of its (a)-failed against 32/111 = 0.288. The largest within-stratum gap anywhere on the grid is 0.251 (`canonical_core_3helix_ncca3`, (a)-passed); most are single-digit points. The headline survives the stratification. Two of the new sabotages initially stayed GREEN, and both were real weaknesses: - the wiring test read the COMMITTED report, which a sabotage does not regenerate, so removing the call left it green ([[artifact-pinning-test-cannot-see-the-code]] — the third time this class has bitten in this repo). The synthetic joint rows now carry an asymmetric (a) split, and the wiring is asserted on a body the test computes. - "absent ⇒ None" was enforced by two guards, so neither could be shown to bite alone. Restructured to one: the isinstance handling coerces (a type question) and a single `if not out: return None` decides absence. 6 new tests (68 in the file), each RED under a named sabotage; harness now 67 sabotages + 2 negative controls, 0 problems. Report regenerated — every previously reported number unchanged, the block is additive. Dev-log stanza records the stratification and one intermediate full-tier run whose 14th failure was `test_report_write_barrier` timing out under concurrent load (passes in isolation on both the worktree and main; the diff touches nothing it exercises). tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures (13 vs 26). ruff + black clean.
… must not be able to skip CodeRabbit CLI round 5 on 4df881e (two rate-limit stalls waited out, no Greptile credit): 1 finding, real, minor. Four artifact tests resolved their report paths against the CURRENT WORKING DIRECTORY and were guarded by `skipif(not path.is_file())`. Run from anywhere but the repo root they skipped — and, worse, a genuinely MISSING committed report would also have skipped. A skip is not a pass, and these four are the only tests that check the committed reports at all: the FP report's four arm strings (the seam's whole safety claim), the comparison's internal consistency, the two arms' shared grid, and that every interval is stated on the record-level n. Paths now resolve from `__file__` (`REPO_ROOT = Path(__file__).resolve().parents[2]`) and every `skipif` is gone. Two new tests assert the preconditions the skips used to swallow: the paths are absolute and the files exist. Verified by running both files from `/tmp` — 69 passed, 0 skipped. The two new sabotages have to run pytest from a foreign cwd to bite, since a cwd-relative path is only wrong when the cwd is wrong; the harness now does that for them. 71 sabotages + 2 negative controls, 0 problems. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/tbox_finder/mining/architecture_param_control_compare.py (1)
486-488: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
nis read with a default of 0 whilefailedis read strictly.Line 486 uses
c_arm.get("n", 0), so a stratum object that omitsnyieldsshare_failed: Noneand reads as "measured and found nothing". Line 487 usesc_arm["failed"], which raisesKeyErrorfor the same class of defect. Treat both keys the same way, or derivenfromfailed + passed, so a malformed stratum refuses instead of reporting a null share.🤖 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 `@src/tbox_finder/mining/architecture_param_control_compare.py` around lines 486 - 488, Update the c_share and f_share calculations to handle missing n and failed fields consistently, preferably by deriving n from failed plus passed or validating both keys before calculating shares. Ensure malformed stratum objects fail rather than producing a null share for missing n.tests/unit/test_architecture_param_control_compare.py (1)
910-917: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the brittle alternation in the
matchpattern.
match="not\n?\\s*objects|are not "encodes a guess about line wrapping in the error text.assert_grids_identicalraises a single-line f-string, somatch="entr\\(ies\\) that are not"is enough and stays readable. A pattern that matches two unrelated phrasings can also pass against the wrong refusal.🤖 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 `@tests/unit/test_architecture_param_control_compare.py` around lines 910 - 917, Simplify the pytest.raises match in test_a_joint_entry_that_is_not_an_object to use the stable single-line phrase “entr(ies) that are not,” escaping the parentheses for regex matching. Remove the brittle alternation and whitespace/newline handling while preserving the CompareError assertion.src/tbox_finder/mining/architecture_param_measure.py (1)
273-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the three SHA-256 helpers.
sha256_ofis now public here, butcurated_control_sample._sha256_ofat lines 1145-1152 still defines a separate chunked implementation. Two implementations of the same provenance hash can drift. Pointcurated_control_sampleat this helper as well.
read_bytesloads the whole file. The current inputs are small JSON reports and Stockholm files, so this is not a problem today. If a large input is ever hashed here, switch to the chunked form.🤖 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 `@src/tbox_finder/mining/architecture_param_measure.py` around lines 273 - 283, Consolidate the provenance hashing by updating curated_control_sample._sha256_of to reuse the public sha256_of helper from architecture_param_measure instead of defining its own chunked implementation. Preserve the existing importer-facing _sha256_of symbol or alias, and leave the current read_bytes implementation unchanged.
🤖 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 `@src/tbox_finder/mining/architecture_param_measure.py`:
- Around line 1236-1256: Extend the output-path validation around the existing
default-output guard to cover every arm’s committed report path, not only
DEFAULT_OUT. Add or reuse a per-arm committed output path on SupplyArm, resolve
paths before comparison, and reject any --arm/output combination where the
selected arm differs from the arm owning the committed path, while preserving
valid same-arm output and the existing error behavior.
---
Nitpick comments:
In `@src/tbox_finder/mining/architecture_param_control_compare.py`:
- Around line 486-488: Update the c_share and f_share calculations to handle
missing n and failed fields consistently, preferably by deriving n from failed
plus passed or validating both keys before calculating shares. Ensure malformed
stratum objects fail rather than producing a null share for missing n.
In `@src/tbox_finder/mining/architecture_param_measure.py`:
- Around line 273-283: Consolidate the provenance hashing by updating
curated_control_sample._sha256_of to reuse the public sha256_of helper from
architecture_param_measure instead of defining its own chunked implementation.
Preserve the existing importer-facing _sha256_of symbol or alias, and leave the
current read_bytes implementation unchanged.
In `@tests/unit/test_architecture_param_control_compare.py`:
- Around line 910-917: Simplify the pytest.raises match in
test_a_joint_entry_that_is_not_an_object to use the stable single-line phrase
“entr(ies) that are not,” escaping the parentheses for regex matching. Remove
the brittle alternation and whitespace/newline handling while preserving the
CompareError assertion.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6656546a-5f77-4280-b834-55c82bf66b84
⛔ Files ignored due to path filters (1)
analyses/phase3_log.qmdis excluded by!**/*.qmd
📒 Files selected for processing (7)
reports/p3/architecture_parameter_control_comparison.jsonreports/p3/architecture_parameter_measurement_control.jsonsrc/tbox_finder/mining/architecture_param_control_compare.pysrc/tbox_finder/mining/architecture_param_measure.pysrc/tbox_finder/mining/curated_control_sizing.pytests/unit/test_architecture_param_control_compare.pytests/unit/test_architecture_param_measure.py
🚧 Files skipped from review as they are similar to previous changes (1)
- reports/p3/architecture_parameter_measurement_control.json
…pe class at every site, not the one named CodeRabbit CLI round 6 on 023e070 (one more rate-limit stall waited out, still no Greptile credit): 1 finding, real, MAJOR. `compare` called `.items()` on four `supply` sub-blocks and `.get()` on `supply` itself without a type check, so an operator-supplied report whose `supply` — or whose `alignment_depth`/`consensus_width` — is a list, a string or null raised AttributeError and exited 1 with a traceback instead of 3 with a refusal. This is the THIRD site of one escape class in this PR: round 1 closed it for status rows, round 2 for `joint` entries, and each time I fixed the site the reviewer named and left the sibling ([[fixed-one-of-two-identical-things]]). So this round closes the class rather than the site: every `supply` access now goes through `supply_block` or `distribution_block`, both of which refuse by name, and a test asserts the property directly against the module's own source — after the helpers are defined, no raw `["supply"]` subscript remains anywhere in the file. 7 new tests (76 in the file), including both arms parametrized (the fp guard was unreachable from the control's test) and both distribution blocks. Each RED under a named sabotage; harness now 72 sabotages + 2 negative controls, 0 problems. The refactor is behaviour-preserving, checked rather than assumed: the regenerated comparison differs from the committed one only in `generated_at_utc` and `git_sha`. Also in this commit: the dev-log stanza's sabotage count corrected to 69 (the count at the time of the round-5 commit, whose message says 71 — the harness reported 69, and a pushed commit is not amended). The final count is 72. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures (13 vs 26). ruff + black clean.
…ued one are not the same CodeRabbit CLI round 7 on 4be5dfd: 2 findings — 1 real (minor), 1 REFUTED (major). REAL. `test_only_the_self_description_changes_between_the_two_arms` walked the two report bodies with `a.get(k)`, which returns None both for an ABSENT key and for a key whose value IS None. An arm that dropped a null-valued field would therefore have looked identical to one that kept it, and the test would have gone on asserting "only four paths differ" straight through a real schema change — which matters, because that test is what makes the "the arm changes nothing measurable" claim checkable. Walks with a sentinel now, plus a test of the sentinel itself (RED under a named sabotage). REFUTED, by measurement rather than by argument. The major claimed reports/p3/architecture_parameter_control_comparison.json and architecture_parameter_measurement_control.json are untracked, so "a clean checkout fails the committed-report tests". Both are tracked, both are in the pushed HEAD tree, both are on origin (28,719 and 50,053 bytes), and a pristine `git clone --depth 1 --branch p3-15g-iv` of the branch runs all 150 tests in the two files GREEN. The finding appears to be reading the diff against `main`, where the files legitimately do not exist yet — which is what a PR adding them looks like. 73 sabotages + 2 negative controls, 0 problems. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean.
… and a read outside the refusal CodeRabbit CLI round 9 on d7b3598: 2 findings, both real, both minor. ⚠ Round 8 read *No new findings* and this round found two — the count went UP, which is why a single clean round is not convergence in this repo. 1 — MY OWN GUARD TEST WAS DISARMABLE. `test_no_supply_access_bypasses_the_validated_ readers`, added in round 6 to close the escape class at every site, scanned only `source.split("def supply_block", 1)[1]`: an access placed ABOVE the reader was never looked at, and `report['supply']` in single quotes passed either way. It now scans every line of the module for both quote styles, and its positive control runs the same predicate over synthetic sources (one offender above the reader, one single-quoted, one clean) rather than over the module, which is and must stay clean. The sabotage is now a MODULE edit — a `["supply"]` access inserted above the reader — because sabotaging the test alone could not make it fail. 2 — `sha256_of` reads each external input, and `build_provenance` hashes the repo-relative ones, AFTER `compare` has returned and outside the try that guards it. `Path(...).is_file()` only proves the entry existed a moment earlier. Wrapped in the same refusal. ⚠ THE TEST FOR (2) IS AN INJECTION, AND THE COMMIT SAYS SO. Every path in the hashing loop is also read inside `compare`, so an unreadable file fails earlier and the earlier refusal masks this one — the reachable case is a TOCTOU. The OSError is therefore injected at exactly that boundary (`monkeypatch` on `sha256_of`) rather than staged with a scenario the CLI cannot reach, and it ships with a positive control asserting the same invocation returns 0 and writes the report. Four sabotage anchors needed repairing after the second `try` block moved code around; two of my new entries were pointing at the wrong file and at a renamed test, which is why they read GREEN before the fix. 75 sabotages + 2 negative controls, 0 problems. The regenerated comparison differs from the committed one only in `generated_at_utc` and `git_sha`. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_architecture_param_control_compare.py (1)
1204-1221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDetect unvalidated
.get("supply")access.Line 1219 detects only subscript access. A future
report.get("supply")outsidesupply_blockbypasses this test and can reintroduce an unvalidated malformed-report path.Use an AST-based check, or whitelist the single getter inside
supply_block, so the guard covers both access forms.🤖 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 `@tests/unit/test_architecture_param_control_compare.py` around lines 1204 - 1221, Update test_no_supply_access_bypasses_the_validated_readers to detect report.get("supply") calls as well as both subscript forms throughout the module source. Prefer an AST-based check that excludes only the approved getter inside supply_block, or explicitly whitelist that occurrence while flagging all other unvalidated access.
🤖 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.
Nitpick comments:
In `@tests/unit/test_architecture_param_control_compare.py`:
- Around line 1204-1221: Update
test_no_supply_access_bypasses_the_validated_readers to detect
report.get("supply") calls as well as both subscript forms throughout the module
source. Prefer an AST-based check that excludes only the approved getter inside
supply_block, or explicitly whitelist that occurrence while flagging all other
unvalidated access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ec915728-b55b-4cf8-828b-a4efe9ecea08
⛔ Files ignored due to path filters (1)
analyses/phase3_log.qmdis excluded by!**/*.qmd
📒 Files selected for processing (4)
reports/p3/architecture_parameter_control_comparison.jsonsrc/tbox_finder/mining/architecture_param_control_compare.pytests/unit/test_architecture_param_control_compare.pytests/unit/test_architecture_param_measure.py
…gin, and a refuted premise pinned as a fact CodeRabbit CLI round 10 on 13638dd: 2 findings — 1 real (minor), 1 REFUTED by measurement. ⚠ The count went 0 → 2 → 2 across the last three rounds. REAL. The `supply_origin` guard — the one field written VERBATIM into a public report — rejected only a leading `/`. `~/work/round_p3_15g_control` expands to the same home directory and `C:\Users\...` carries the account name just as plainly, and both passed. The predicate is now `is_local_path_shaped` in `architecture_param_measure`, used by BOTH modules that carry this field, because a guard fixed at one of two identical sites is this PR's recurring defect. Parametrized over all three local spellings and over three legitimate host-qualified values (the real one contains a colon and must not be caught). REFUTED. The other finding said `test_a_non_default_arm_with_the_default_out_is_refused` could pass for the wrong reason: no `reports/p3` under `tmp_path`, so a rc of 3 might come from the write failing rather than from the guard. Measured all four cells — guard present: PASS; guard removed with the directory created: FAIL; guard removed with the directory ABSENT: FAIL. It cannot pass for that reason, because `main` calls `out.parent.mkdir(parents=True, exist_ok=True)` and creates the directory itself. I had already applied the suggested `mkdir` before measuring. It is reverted: keeping it would have left a comment in the test asserting a rationale the code disproves. In its place, `test_main_creates_its_own_output_parent_directory` pins the behaviour the refutation rests on, so if `main` ever stops creating its parent, that test goes red before the arm/out test can start passing for the wrong reason. ⚠ Two of this round's new sabotage entries read GREEN at first and neither was a code defect: the harness compared node ids exactly, so `pytest.mark.parametrize`'s `[param]` suffix never matched, and one entry pointed at the source module while its anchor was test-file text. Both fixed in the harness. 79 sabotages + 2 negative controls, 0 problems. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean.
…and a repeat refutation re-measured CodeRabbit CLI round 11 on 64f6dbc: 2 findings, both MAJOR — 1 real, 1 a repeat of round 7's refuted claim. REAL. Round 10's `is_local_path_shaped` still missed two spellings, and the reviewer also caught WHY my matrix hid them: `C:/Users/x/msa` (drive letter with FORWARD slashes) and `\\server\share\msa` (UNC with BACKslashes) are neither POSIX-absolute nor matched by `":\\" in text`, and the case I had written as "the UNC form", `//server/share/msa`, is already absolute under a POSIX `Path` — so it passed without ever exercising a Windows arm. The predicate now spells each form out, and the matrix carries all six. Each of the four arms is sabotaged SEPARATELY, because an arm that is redundant with another cannot be shown to bite. REFUTED, second time, re-measured at the exact pushed HEAD rather than by citing the earlier refutation. Both reports are on origin at `p3-15g-iv` (28,719 and 50,053 bytes), and a pristine clone of `64f6dbc` runs all 169 tests in the two files GREEN — including `test_all_three_committed_reports_are_present_at_all`, the test the finding says fails. The claim is an artifact of diffing against `main`, where files a PR ADDS do not exist yet. 81 sabotages + 2 negative controls, 0 problems. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean.
…views of 023e070 and 13638dd ⚠ THESE WERE UNREAD. The CodeRabbit GitHub app had reviewed two later commits while I was working the CLI rounds, and I only noticed when the CLI started erroring. Five findings, all real. Read every review the app posts, not only the first ([[review-round-judged-from-truncated-buffer]]). 1 (actionable) — THE ARM/OUT GUARD WAS ONE-DIRECTIONAL. It refused (non-default arm → the FP report's path) and said nothing about the reverse: `--arm round0_fp --out reports/p3/architecture_parameter_measurement_control.json` writes FP numbers and FP prose over the committed CONTROL artifact — which is now an input to architecture_param_control_compare, so the damage is the same class. Each arm now declares its own `canonical_out` on its `SupplyArm`, and the CLI refuses to write any arm's numbers to any OTHER arm's canonical path, in both directions. A test asserts the paths are distinct, because two arms sharing one would make the guard vacuous. 2 — `stratify_by_criterion_a` read `n` as `.get("n", 0)` while reading `failed` strictly, so a stratum missing `n` published `share_failed: None` — "measured and found nothing" — where the same defect in `failed` raised. All three counts are now required AND `n` is checked against `failed + passed`. 3 — a test's `match="not\n?\\s*objects|are not "` encoded a guess about line wrapping and could match two unrelated refusals. Replaced with the precise pattern. 4 — `sha256_of` was a THIRD implementation of the same provenance hash, alongside two chunked copies in curated_control_sample and curated_control_run. Consolidated — but in the opposite direction to the suggestion: the shared helper is now the CHUNKED form, because two of those call sites hash a model CHECKPOINT and `read_bytes()` would have traded a duplication for a memory regression. The test asserts the digest matches an independent one-shot hash and that the file is read in >1 chunk, rather than asserting the three modules agree — they delegate now, so that would be a tautology ([[promote-dont-duplicate-is-a-correctness-rule]]). 5 — the round-6 source scan detected only SUBSCRIPT access, so a future `report.get("supply")` outside `supply_block` would reintroduce the unvalidated path. It now covers both access forms, with the reader's own getter whitelisted by LINE SPAN rather than by text (a text whitelist would match a copy of it anywhere). All three reports regenerate identically (control, comparison, and the FP report still byte-identical across the whole seam). 87 sabotages + 2 negative controls, 0 problems; three anchors needed repairing after the guard rewrite. tests/unit + tests/golden vs the matched main baseline at c1a5b97: ZERO new failures. ruff + black clean. ⚠ The CodeRabbit CLI now fails with "A valid organization session is required" (auth status still shows a Pro+ seat assigned). The GitHub app path remains rate-limited.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tbox_finder/mining/architecture_param_measure.py (1)
1341-1341: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle external-input hash failures with the refusal convention.
Line 1341 runs outside a
tryblock.Path(candidate).is_file()can succeed before the external file is removed or becomes unreadable.sha256_of()then raisesOSError, and the CLI exits with a traceback instead of returning exit code 3.Wrap the provenance-input loop and
build_provenance()call in anOSErrorrefusal block. The comparison CLI already protects the equivalent hashing path.Proposed fix
- for label, candidate in ( - ("manifest", args.manifest), - ("covariation_status", args.covariation_status), - ("positive_control", args.positive_control), - ): - ... - external[label] = {"name": Path(candidate).name, "sha256": sha256_of(candidate)} - body["provenance"] = build_provenance(...) + try: + for label, candidate in ( + ("manifest", args.manifest), + ("covariation_status", args.covariation_status), + ("positive_control", args.positive_control), + ): + ... + external[label] = { + "name": Path(candidate).name, + "sha256": sha256_of(candidate), + } + body["provenance"] = build_provenance(...) + except OSError as exc: + print(f"refused: {exc}", file=sys.stderr) + return 3🤖 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 `@src/tbox_finder/mining/architecture_param_measure.py` at line 1341, Wrap the provenance-input loop and build_provenance() call in an OSError handler so failures from sha256_of() or related external-input access return the CLI’s refusal convention with exit code 3 instead of propagating a traceback. Preserve the existing external metadata construction and comparison CLI behavior.
🤖 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.
Outside diff comments:
In `@src/tbox_finder/mining/architecture_param_measure.py`:
- Line 1341: Wrap the provenance-input loop and build_provenance() call in an
OSError handler so failures from sha256_of() or related external-input access
return the CLI’s refusal convention with exit code 3 instead of propagating a
traceback. Preserve the existing external metadata construction and comparison
CLI behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c2df7c1a-8ecb-4aa4-a614-148d5a6a0712
📒 Files selected for processing (6)
src/tbox_finder/mining/architecture_param_control_compare.pysrc/tbox_finder/mining/architecture_param_measure.pysrc/tbox_finder/mining/curated_control_run.pysrc/tbox_finder/mining/curated_control_sample.pytests/unit/test_architecture_param_control_compare.pytests/unit/test_architecture_param_measure.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/test_architecture_param_control_compare.py
- src/tbox_finder/mining/architecture_param_control_compare.py
…d sabotage count Prose only (`.coderabbit.yaml` scopes review to code-that-runs, so this diff correctly draws no findings). - sabotage count corrected to the final 87; two earlier commit messages quote intermediate counts that were wrong when written, and a pushed commit is not amended, so the stanza is the number to read. - the review record: 13 CodeRabbit CLI rounds and 3 GitHub-app reviews, 26 findings, 24 real (4 major), 2 refuted by measurement; six rate-limit stalls all waited out, no Greptile credit spent. - the three things worth carrying out of this step: two app reviews sat unread for hours while I worked the CLI; one defect class took four rounds to close because I kept fixing the named site instead of the class; and the guard-test enforcing that class was itself defective twice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P3-15′-fmeasured criterion (b)'s seven ADR-0006 A4 rule parameters on 278 de-novo consensuses of round-0 FP-manifest candidates — a population of unknown status — and the §7 ruling was "measure a matched control first".P3-15′-g/-g-ii/-g-iiibuilt it (SLURM job 1264: 76 consensuses of 317 Stage-1 queries over 160 held-out curated records). This step measures the control at P3-15′-f's identical grid and reads the two arms against each other.The result
(b)'s share of
failedamong decided candidates:loosest_nonvacuoussensitive_corecanonical_core_3helixcanonical_core_3helix_ncca3curated_freeze_transplantstrictestThe largest gap on the named grid is 14.7 pp; at three of six settings the control fails more than the FP arm; at 5 of 6 the control's record-level interval contains the FP point. (b) is behaving as a severity knob on both populations, not as a discriminator between them.
⚠ The largest threat to that reading is stated in the report itself: the FP arm is a false-positive manifest, not a verified-negative set.
Every interval is on the record-level n
149 of the 160 records contribute 2 queries and 4 contribute 3, so
control_query_level.ci95isnullin every row with the reason beside it. The CI is n = 68 producible records throughout, exactly asimp.mdrequired — never the 317 queries.The seam, and why it changes nothing measurable
architecture_param_measuregains a frozenSupplyArmregistry (--arm) supplying only the four strings a report makes about itself (step,disclosure, criterion (a)'scontrol_note, the provenance rule). Shipping the FP arm's prose over the control's numbers would be a committed public report describing the wrong corpus, and no internal check could see it because every count would still reconcile.--arm round0_fpis the default and the committed FP report re-derives byte-identically across the seam (verified against the retrieved supply; onlygenerated_at_utc,git_sha,env_lock_hashdiffer).candidate_stateis extracted fromevaluate_tuple's loop body so the record-level aggregation reads the same code the counts come from.Also measured
(a)-passedcannot be read as an enrichment for T-box identity.pins_nothing: truein both reports. The ADR-0006 A4 seven-parameter choice stays the open §7 decision; this step informs it and does not take it.Validation
tests/unit+tests/goldenvs a matchedmainbaseline atc1a5b97: zero new failures — worktree 13 FAILED, main 26, a strict subset. The whole delta is accounted for: 14 tests move to SKIPPED on unmaterialised DVC/LFS inputs, and passes rise by exactly the 54 tests added. Both^FAILEDsets ANSI-stripped and cross-checked against the summary line.anyandallagreed) — fixed by rebuilding the fixtures, not the sabotages.generated_at_utc.ruff+blackclean.No SLURM job. No ADR amendment. No DVC object.
Summary by CodeRabbit