P3-15'-f: re-derive criterion (b)'s seven rule parameters from the matched control - #126
Merged
Merged
Conversation
…ers from the matched control P3-15'-f measured criterion (b) on 278 de-novo consensuses of round-0 false-positive-manifest candidates and put seven ADR-0006 A4 rule parameters to the user, who declined to pin them on that evidence and asked for a matched positive control first. P3-15'-g/-g-ii/-g-iii/-g-iv built and measured it. P3-15'-f's block asked for the recommendation to be re-derived, not re-quoted, once the control landed. This is that re-derivation, over the whole grid rather than the six named tuples. Inputs - two.amlab:$HOME/tbox-scratch/round_p3_15_supply/msa (278, jobs 1205+1254, 90b2fe1b...) - two.amlab:$HOME/tbox-scratch/round_p3_15g_control/msa (76, job 1264, 0b5afbec...) - criterion (a) 941-row status (3215f034..., the digest the FP measurement records) - criterion (c) 941-row status (3c8ac8e3..., the digest the exclusion diagnostic records) - P3-15'-b's 941 Stage-2 posteriors (carried as a sensitivity; see below) Outputs - src/tbox_finder/mining/architecture_param_recommend.py - reports/p3/architecture_parameter_recommendation.json (pins_nothing: true) - data/processed/mining/round0_fp_spare_rule_inputs_v0.json (the committed 941-row join) - tests/unit/test_architecture_param_recommend.py (69 tests) - analyses/phase3_log.qmd stanza The headline is not about (b): the round's yield ceiling is 15 of 941 Sparing is a disjunction, so mining is a conjunction. Criterion (a) fails on 111 of 941 and criterion (c) on 96; their intersection is 15, and all 15 have a consensus. Declaring the Stage-2 disjunct lowers it to 12 (thr 0.5) or 13 (0.9/0.95/0.99). Over the entire admissible grid the round mines between 1 and 13. (b)'s seven parameters are choosing among single-digit differences, which no committed report stated. The rule is stated before the counts Four floors are read off ADR-0006 D3's wording and ADR-0004 D1's vocabulary (min_named_helices >= 2, min_helix_pairs >= 2, ncca_pairing_nt >= 2, a bounded bulge_max_nt); among the 864 survivors the rule takes the most permissive as measured, which is A4's own fail-closed argument for a sparing disjunct. That selects exactly sensitive_core, tied only with its proved-inert allow_wobble twin. Admissibility is MEASURED by running the shipped localizer (2,880 of 3,840 points; the 960 refusals are one rule), and each point's yield goes through the shipped spare_rule.is_mining_excluded rather than a re-derived conjunction. The composite (a) AND (b) is anti-selective at the strict end -g-iv compared (b) alone and found no separation. On the conjunction the control falls through MORE often than the FP arm at five of six named settings (9.2 vs 7.2 %, 10.5 vs 11.5 %, 38.2 vs 23.7 %, 43.4 vs 32.0 %, 57.9 vs 37.4 %, 59.2 vs 38.5 %), with disjoint 95 % Wilson intervals at the two strictest. (a) passes 39.5 % of the control against 60.1 % of the FP arm, so tightening (b) removes protection where (a) protected least. Disclosures - The report enumerates the 8 floor-clearing settings that beat its own recommendation (at most 2 extra mined) and states why the rule declines them. - Criterion (c) is 546/96/299, not the 541/101/299 TODO.md and imp.md record. - No committed artifact anchors the Stage-2 posteriors' digest and a second scoring exists differing by up to 3.8e-2; that column is a sensitivity, and the ceiling of 15 uses (a) and (c) alone. - slurm/p3/stage1_remine.sbatch never passes --relaxed-arch-status/--synteny-status, so remine.py:512 refuses a round declaring either backend live. Reported, not patched. Validation Zero new failures vs a matched main baseline at 090547f (worktree 13 FAILED, main 26, a strict subset; the delta is DVC/LFS skips, named individually). 40 sabotages all RED against their NAMED test plus 2 green negative controls; two defects of this step were caught by that harness, not by review (a duplicated floor filter, and a vacuously-true identity clause). Both artifacts re-derive byte-identically apart from generated_at_utc. ruff + black clean. pins_nothing: true; no ADR amended. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a complete architecture-parameter recommendation workflow. It validates provenance-bound inputs, evaluates the shipped localizer across a parameter grid, measures control and false-positive outcomes, selects a floor-clearing setting, and writes a non-pinning report. ChangesArchitecture recommendation
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🔵 Low · up to The PR is mergeable with explicit owner awareness: a test fixture can miss certain counter inversions, and another test does not exercise the JSON null case it names. These are bounded validation gaps with no supplied evidence of a production behavior defect. Sequence Diagram(s)sequenceDiagram
participant CLI
participant build_inputs
participant recommend
participant evaluate_point
participant JSONReport
CLI->>build_inputs: join source tables and record provenance
build_inputs-->>CLI: write serialized inputs
CLI->>recommend: validate manifests, supplies, and digests
recommend->>evaluate_point: evaluate parameter grid
evaluate_point-->>recommend: return yield and control measurements
recommend->>JSONReport: assemble recommendation and ceilings
JSONReport-->>CLI: write JSON report
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 6
🧹 Nitpick comments (7)
src/tbox_finder/mining/architecture_param_recommend.py (4)
1266-1269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
encoding="utf-8"towrite_text.Every read in this module pins
encoding="utf-8". This write relies on the platform default.json.dumpskeepsensure_ascii=Trueby default, so the current output is ASCII and safe. If someone later setsensure_ascii=False, the em dashes and⚠in the emitted rationales would raiseUnicodeEncodeErroron a non-UTF-8 locale.♻️ Proposed change
- out.write_text(json.dumps(body, indent=2, sort_keys=True) + "\n") + out.write_text(json.dumps(body, indent=2, sort_keys=True) + "\n", encoding="utf-8")🤖 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_recommend.py` around lines 1266 - 1269, Update the _write function’s out.write_text call to explicitly use encoding="utf-8", matching the module’s existing read behavior and ensuring future non-ASCII JSON output is written reliably.
791-794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMeasured counts are hardcoded in prose the report publishes.
The docstring states "2,880 points" and "68 records".
DECISION_RULE_STATEMENT(Line 229) andwhy_the_rule_declines_them(Lines 822-824) carry the same literals, andrecommendemits both strings verbatim into the report.n_admissibleandn_records_producibleare computed at run time. If the supply changes, the report prints stale counts beside the correct derived ones, and a reader cannot tell which is which.Format these counts from the computed values, or move them out of the published strings.
🤖 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_recommend.py` around lines 791 - 794, Update DECISION_RULE_STATEMENT, why_the_rule_declines_them, and the report assembly in recommend to avoid hardcoded “2,880 points” and “68 records”; interpolate the computed n_admissible and n_records_producible values wherever those published strings mention the counts, or remove the counts from the reusable prose and emit them from the runtime data.
112-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
DECIDED_STATESfrom the imported status constants.The module imports
STATUS_PASSEDandSTATUS_FAILEDbut re-spells the same two values as literals here. A change to those constants inspare_rulewould leave this tuple stale, and every guard that reads it (control_damage,assert_supply_is_the_decided_set) would silently classify records as not producible.♻️ Proposed change
-DECIDED_STATES: tuple[str, ...] = ("passed", "failed") +DECIDED_STATES: tuple[str, ...] = (STATUS_PASSED, STATUS_FAILED)🤖 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_recommend.py` around lines 112 - 113, Update DECIDED_STATES to derive its tuple values from the imported STATUS_PASSED and STATUS_FAILED constants instead of duplicating string literals, preserving the existing ordering and consumers such as control_damage and assert_supply_is_the_decided_set.
950-954: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAdd an explicit slug-cardinality check.
candidate_sluguses a 12-hex (48-bit) digest prefix, so it is not strictly injective. A collision would overwrite an entry in either index. The current candidate sets have no collisions, but compare each index size with its candidate-id count before evaluation.🤖 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_recommend.py` around lines 950 - 954, Add explicit cardinality checks after building fp_id_by_slug and control_id_by_slug, comparing each index’s size with its corresponding candidate-ID count before evaluation. Reject or fail clearly when the counts differ, preserving all entries only when candidate_slug values are unique.tests/unit/test_architecture_param_recommend.py (3)
106-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead
if Falsebranch.
p.as_dict()["label"] if False else ...always takes theelsearm. The condition is a debug artifact. The dead arm is also wrong:as_dict()omitslabel, as Line 738 shows by passinglabelseparately, so enabling the branch would raiseKeyError.♻️ Proposed change
- assert ( - len({p.as_dict()["label"] if False else tuple(sorted(p.as_dict().items())) for p in points}) - == expected - ) + assert len({tuple(sorted(p.as_dict().items())) for p in points}) == expected🤖 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_recommend.py` around lines 106 - 109, Remove the dead conditional expression from the set comprehension in the architecture parameter assertion, using the sorted `p.as_dict().items()` tuple directly. Preserve the existing deduplication and comparison against expected.
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstruct
ParamTuplewith keyword arguments in the fixtures.
LOOSEandCHOSENpass seven positional values, and about twenty later call sites repeat the pattern. The axis order isstem_i_nt_threshold, min_named_helices, min_helix_pairs, bulge_min_nt, bulge_max_nt, ncca_pairing_nt, allow_wobble, which a reader must reconstruct fromgrid_points. If a field is reordered inParamTuple, every fixture silently repoints at a different axis, the floor tests keep passing, and each one then asserts about a parameter it does not name.♻️ Proposed change
-LOOSE = apm.ParamTuple("loose", 1, 1, 2, 1, 10_000, 1, False) -CHOSEN = apm.ParamTuple("sensitive_core", 1, 2, 2, 2, 50, 2, False) +LOOSE = apm.ParamTuple( + label="loose", + stem_i_nt_threshold=1, + min_named_helices=1, + min_helix_pairs=2, + bulge_min_nt=1, + bulge_max_nt=10_000, + ncca_pairing_nt=1, + allow_wobble=False, +) +CHOSEN = apm.ParamTuple( + label="sensitive_core", + stem_i_nt_threshold=1, + min_named_helices=2, + min_helix_pairs=2, + bulge_min_nt=2, + bulge_max_nt=50, + ncca_pairing_nt=2, + allow_wobble=False, +)🤖 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_recommend.py` around lines 45 - 46, Update the LOOSE and CHOSEN fixtures, along with the repeated ParamTuple call sites in this test module, to pass all fields by their parameter names rather than positionally. Use the existing axis names—stem_i_nt_threshold, min_named_helices, min_helix_pairs, bulge_min_nt, bulge_max_nt, ncca_pairing_nt, and allow_wobble—while preserving each fixture’s current values and behavior.
331-336: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the case the tie refusal does not catch.
This test pins the refusal for two settings that share a
_tie_break_key. It does not cover two settings inargminthat differ in a non-inert parameter and also differ inbulge_max_nt. That pair is resolved silently by sort order today. Refer to the comment onapply_decision_ruleatsrc/tbox_finder/mining/architecture_param_recommend.pyLines 656-673 for the underlying defect.💚 Proposed additional test
def test_a_tie_across_a_non_inert_axis_is_refused_even_when_bulge_max_differs(): """Sort order must not decide min_helix_pairs just because bulge_max_nt differs.""" a = point(params=apm.ParamTuple("a", 1, 2, 2, 2, 20, 2, False), fp_failed=10) b = point(params=apm.ParamTuple("b", 1, 2, 3, 2, 50, 2, False), fp_failed=10) with pytest.raises(rec.RecommendError, match="tied"): rec.apply_decision_rule([a, b])🤖 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_recommend.py` around lines 331 - 336, Add a regression test beside test_a_tie_the_break_order_cannot_resolve_is_refused covering equal fp_failed candidates whose non-inert parameter and bulge_max_nt differ, such as the proposed ParamTuple values. Assert apply_decision_rule raises RecommendError matching “tied”, ensuring sort order cannot silently resolve min_helix_pairs.
🤖 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_recommend.py`:
- Around line 1151-1152: The manifest candidate IDs are not validated for
duplicates, allowing incorrect corpus cardinality and coverage checks. In
src/tbox_finder/mining/architecture_param_recommend.py lines 1151-1152, reject
duplicate manifest_ids before constructing rows; in lines 337-346, compare the
unique-ID count with the total count before the existing set-equality check so
duplicates cannot pass coverage validation.
- Around line 1151-1152: Deduplicate candidate IDs when constructing
manifest_ids in the manifest-loading flow, before the downstream sorted rows and
n_candidates calculations. Reuse the existing candidate_id_of-based extraction
and preserve deterministic ordering for unique IDs, so duplicate manifest
entries cannot inflate the candidate count or trigger a misleading
load_spare_rule_inputs failure.
- Around line 184-191: The rationale in architecture_param_recommend.py around
the bulge_max_nt recommendation must not claim verification through
recommendation.floor_sensitivity, because that field is None when the selection
does not move; remove that cited clause or directly measure and report the
sentinel comparison through an appropriate field. Regenerate
reports/p3/architecture_parameter_recommendation.json so its published rationale
and floor_sensitivity data match the source change.
- Around line 656-673: The tie refusal in _tie_break_key handling must detect
argmin settings that differ on any parameter outside TIE_BREAK_ORDER, even when
their tie-break keys differ; update the selection logic around chosen and
still_tied to reject such unresolved non-inert differences instead of relying on
sort order. In src/tbox_finder/mining/architecture_param_recommend.py lines
656-673, preserve normal tie-break behavior only when differences are limited to
the ordered vocabulary. In tests/unit/test_architecture_param_recommend.py lines
331-336, add coverage for tied settings differing in both min_helix_pairs and
bulge_max_nt and assert RecommendError.
- Around line 328-330: Update the stage2_posterior validation in the
row-processing logic to reject booleans and any numeric value outside the
inclusive range [0, 1]. Preserve acceptance of valid int and float
probabilities, and raise the existing RecommendError with the row path and
candidate ID for invalid values.
- Around line 474-508: Update control_damage so the zero-record case is handled
before calling wilson_interval or dividing by n_producible. When no records
contain producible queries, raise the established RecommendError instead of
producing confidence intervals or shares; preserve the existing calculations for
n_producible greater than zero.
---
Nitpick comments:
In `@src/tbox_finder/mining/architecture_param_recommend.py`:
- Around line 1266-1269: Update the _write function’s out.write_text call to
explicitly use encoding="utf-8", matching the module’s existing read behavior
and ensuring future non-ASCII JSON output is written reliably.
- Around line 791-794: Update DECISION_RULE_STATEMENT,
why_the_rule_declines_them, and the report assembly in recommend to avoid
hardcoded “2,880 points” and “68 records”; interpolate the computed n_admissible
and n_records_producible values wherever those published strings mention the
counts, or remove the counts from the reusable prose and emit them from the
runtime data.
- Around line 112-113: Update DECIDED_STATES to derive its tuple values from the
imported STATUS_PASSED and STATUS_FAILED constants instead of duplicating string
literals, preserving the existing ordering and consumers such as control_damage
and assert_supply_is_the_decided_set.
- Around line 950-954: Add explicit cardinality checks after building
fp_id_by_slug and control_id_by_slug, comparing each index’s size with its
corresponding candidate-ID count before evaluation. Reject or fail clearly when
the counts differ, preserving all entries only when candidate_slug values are
unique.
In `@tests/unit/test_architecture_param_recommend.py`:
- Around line 106-109: Remove the dead conditional expression from the set
comprehension in the architecture parameter assertion, using the sorted
`p.as_dict().items()` tuple directly. Preserve the existing deduplication and
comparison against expected.
- Around line 45-46: Update the LOOSE and CHOSEN fixtures, along with the
repeated ParamTuple call sites in this test module, to pass all fields by their
parameter names rather than positionally. Use the existing axis
names—stem_i_nt_threshold, min_named_helices, min_helix_pairs, bulge_min_nt,
bulge_max_nt, ncca_pairing_nt, and allow_wobble—while preserving each fixture’s
current values and behavior.
- Around line 331-336: Add a regression test beside
test_a_tie_the_break_order_cannot_resolve_is_refused covering equal fp_failed
candidates whose non-inert parameter and bulge_max_nt differ, such as the
proposed ParamTuple values. Assert apply_decision_rule raises RecommendError
matching “tied”, ensuring sort order cannot silently resolve min_helix_pairs.
🪄 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: 790a789a-6a7b-4b7f-ab64-b9a057850c75
⛔ Files ignored due to path filters (2)
analyses/phase3_log.qmdis excluded by!**/*.qmddata/processed/mining/round0_fp_spare_rule_inputs_v0.jsonis excluded by!data/**
📒 Files selected for processing (3)
reports/p3/architecture_parameter_recommendation.jsonsrc/tbox_finder/mining/architecture_param_recommend.pytests/unit/test_architecture_param_recommend.py
…rd the empty denominator Two CodeRabbit CLI findings, both real. 1. The bounded-bulge floor's rationale cited recommendation.floor_sensitivity for the claim that keeping the bound costs nothing. That field is null in exactly this case (the floor does not move the selection, so the "identical" comparison would be the chosen setting against itself), so the citation pointed at the one place it could not be read. bulge_sentinel_comparison now measures the chosen setting against its own bulge_max_nt = 10000 twin and the rationale cites that: identical on fp_failed (52), n_mined (3), records losing (b) (9) and records losing (a) AND (b) (8). 2. control_damage divided by n_producible and called wilson_interval on it with no guard, so a control whose every record is unproducible raised SizingError / ZeroDivisionError instead of RecommendError — while anti_selectivity already guarded the same input. Both paths now refuse by name. Six tests added (75 total), each with a positive control; four sabotages added (44 total), all RED against their named test plus the two green negative controls. Report regenerated: the recommendation, the ceiling and the frontier are unchanged. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
…ifest ids, posterior validation Five CodeRabbit GitHub-app findings; two duplicated the CLI round already fixed in b480caf, three are new and all real. 1. MAJOR — the tie refusal covered three of the seven parameters. It fired only when two argmin members shared a _tie_break_key, so two settings differing in BOTH min_helix_pairs and bulge_max_nt have different keys and were separated by bulge_max_nt alone, publishing a min_helix_pairs the rule never chose. That contradicts step (4) of DECISION_RULE_STATEMENT. The refusal now asks whether the tied settings disagree on any parameter outside TIE_BREAK_VOCABULARY, which is derived from TIE_BREAK_ORDER rather than restated beside it. ⚠ The recommendation, the yield ceiling and the frontier are unchanged. The only movement in the regenerated report is floor_sensitivity reporting 3 and 7 tied settings where it reported 2 and 4 — the old check had been resolving ties it should have refused. 2. MAJOR — a duplicated manifest candidate_id was never refused. Every corpus size in this report is a row count and every join against it is a set, so a duplicate inflates the denominator while assert_covers_manifest still reads as exact. Refused now in both assert_covers_manifest and build_inputs. 3. MINOR — stage2_posterior accepted bool (bool subclasses int, so `true` reads as 1.0 and SPARES at every threshold) and accepted values outside [0, 1] (a mis-scaled column would clear every threshold without a refusal). Both refused. Seven tests added (82 total), each with a positive control; five sabotages added (49 total), all RED against their named test plus two green negative controls. Zero new failures vs the matched main baseline at 090547f. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
Owner
Author
|
| Filename | Overview |
|---|---|
| src/tbox_finder/mining/architecture_param_recommend.py | Adds the recommendation pipeline; report-derived supply origins bypass the local-path validation used for the explicit origin argument. |
| tests/unit/test_architecture_param_recommend.py | Adds broad coverage for grid admissibility, spare-rule conjunctions, decision floors, tie handling, input validation, and end-to-end report generation. |
| reports/p3/architecture_parameter_recommendation.json | Adds the generated recommendation artifact with provenance, yield ceilings, control comparisons, frontiers, and the selected parameter tuple. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Manifest and status artifacts] --> B[Build validated spare-rule inputs]
C[FP consensus supply] --> D[Evaluate admissible parameter grid]
E[Matched control supply] --> D
B --> D
D --> F[Apply semantic floors]
F --> G[Select most permissive measured setting]
G --> H[Recommendation report]
D --> I[Yield and control frontiers]
I --> H
Prompt To Fix All With AI
### Issue 1
src/tbox_finder/mining/architecture_param_recommend.py:1155-1164
**Report origins bypass path validation**
If an FP or control measurement report contains a local filesystem path in `supply.supply_origin`, `recommend()` copies it verbatim into the public recommendation even though the explicit `supply_origin` argument rejects such paths, exposing host-specific paths or account names. Apply the same validation to both report-derived origins before including them. **How this was verified:** The two report fields flow directly into `arms.fp` and `arms.control` without passing through `is_local_path_shaped()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(mining): P3-15'-f review round 2 — t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1155
to
+1164
| "supply_origin": fp_report.get("supply", {}).get("supply_origin"), | ||
| "ground_truth": ( | ||
| "unknown — a round-0 false-positive MANIFEST, not verified negatives" | ||
| ), | ||
| }, | ||
| "control": { | ||
| "step": "P3-15'-g-iv", | ||
| "n_consensuses": len(control_items), | ||
| "supply_digest_sha256": control_digest, | ||
| "supply_origin": control_report.get("supply", {}).get("supply_origin"), |
There was a problem hiding this comment.
If an FP or control measurement report contains a local filesystem path in supply.supply_origin, recommend() copies it verbatim into the public recommendation even though the explicit supply_origin argument rejects such paths, exposing host-specific paths or account names. Apply the same validation to both report-derived origins before including them. How this was verified: The two report fields flow directly into arms.fp and arms.control without passing through is_local_path_shaped().
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tbox_finder/mining/architecture_param_recommend.py
Line: 1155-1164
Comment:
**Report origins bypass path validation**
If an FP or control measurement report contains a local filesystem path in `supply.supply_origin`, `recommend()` copies it verbatim into the public recommendation even though the explicit `supply_origin` argument rejects such paths, exposing host-specific paths or account names. Apply the same validation to both report-derived origins before including them. **How this was verified:** The two report fields flow directly into `arms.fp` and `arms.control` without passing through `is_local_path_shaped()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
…d too, and the floor level was never mine to set A 5-lens adversarial workflow, Greptile and a second CodeRabbit pass between them broke two of this step's published claims. Both are corrected, not hedged. 1. THE FP ARM IS PSEUDO-REPLICATED. Of the 278 (a)-decided FP candidates only 199 are distinct loci: 79 loci contribute two candidate ids differing solely in the tiling window offset, with byte-identical msa.sto, identical (a) and (c) status and identical (b) verdicts at every named setting (verified here). The old unit_caveat stated the asymmetry BACKWARDS — it claimed the control was the pseudo-replicated arm and the FP arm was not — so every FP interval was too narrow while the control was refused one for a property both arms had. locus_of() collapses the window offset and every FP interval is now on n = 199 loci. 2. "DISJOINT INTERVALS AT THE TWO STRICTEST" IS WITHDRAWN. It was computed from a query-level control interval — the one control_damage itself refuses to publish — against a candidate-level FP one. On records (68) vs loci (199) NO setting has disjoint intervals. The control still falls through more at five of six settings, so the direction and the reading are unchanged; the separation claim is not. 3. THE min_named_helices LEVEL IS A §7 CHOICE, NOT A DERIVATION. D3 states no number and delegates the value to P6; a second reading is stated by this repo's own localizer (architecture.named_elements_present: "the canonical class-I core is Stem I + Stem III + antiterminator"), i.e. >= 3. The rule now runs at EVERY level and prices each: >=1 undecidable; >=2 sensitive_core (3 mined / 8 of 68); >=3 mnh3·mhp2· bulge2-50·ncca2 (11 mined / 24 of 68); >=4 mnh4·mhp2·bulge2-20·ncca2 (13 / 31 of 68). Also fixed: the min_helix_pairs floor cited D3 for a concept D3 does not contain (its source now says it is an assumption of this module, and the stack-depth distribution it appeals to is published beside it); control_records derived the record grouping from the candidate-id string, which record_of's own docstring forbids, and now reads accession from the manifest via the shipped load_control_records; a supply_origin carried over from either measurement report bypassed the is_local_path_shaped guard the flag gets (Greptile P2); DECIDED_STATES is derived from the status constants; write_text is explicit about utf-8; and no measured count is written into a published prose string. UNCHANGED BY ALL SIX CORRECTIONS: the yield ceiling of 15, the 1..13 range over the grid, the level-2 selection, and the frontier — counts of candidates and loci, none of which rested on the broken claims. 98 unit tests; 62 sabotages all RED against their NAMED test + 2 green negative controls. Two sabotages initially stayed green and both were my targeting, not the code — a dedicated locus test and a retarget fixed them. Zero new failures vs main at 090547f. Report re-derives byte-identically apart from generated_at_utc. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
…uous FP fixture, and anchors that are checked The 5-lens adversarial workflow returned (38 agents, 0 errors). Three more real corrections, one of them on code this step had just written. 1. THE FP NUMERATOR HAD NO DISCRIMINATING TEST. evaluate_point's fp_with_a — the whole FP side of the anti-selectivity reading — was reached by exactly one fixture, and that fixture set criterion (a) to `failed` on every candidate. Dropping the (a) conjunct, or reading synteny_status instead, left the suite green while the published claim went 5-of-6 -> 1-of-6, or was silently STRENGTHENED to 6-of-6. The locus test now uses an asymmetric corpus (a third candidate that fails (b) but passes (a)) so all three implementations disagree, and both mutations are in the sabotage set. 2. THE CEILING IS 15 MANIFEST ROWS BUT 12 DISTINCT LOCI. The tiling duplication that broke the FP intervals also inflates the ceiling: 3 of the 15 rows are second calls on a locus already counted. Every yield now carries both units; the recommendation's 3 mined rows are 3 distinct loci. 3. build_inputs ASSERTED PROVENANCE IT HAD NOT CHECKED. Its anchored_by strings were unconditional constants that would keep claiming "report X records this digest" after either file changed. anchor_of now reads the named report, compares the digest and returns None on a miss; the (a) and (c) columns verify, the Stage-2 column stays honestly unanchored. min_mined_if_b_passed_everywhere is relabelled as the structural identity it is rather than a measurement. 104 unit tests; 69 sabotages all RED against their NAMED test + 2 green negative controls. Zero new failures vs main at 090547f. Report re-derives byte-identically. Still open and disclosed in the dev-log, not fixed here: frontier's nominal CI on a minimum selected over ~1,000 points; the control damage share being conditioned on producibility (68 producible, not the 160 drawn); the comparison report carried as prose without a digest binding. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
min_named_helices level 2 => sensitive_core (user, AskUserQuestion, 2026-08-12): STEM_I_NT_THRESHOLD=1 · MIN_NAMED_HELICES=2 · MIN_HELIX_PAIRS=2 · BULGE_MIN_NT=2 · BULGE_MAX_NT=50 · NCCA_PAIRING_NT=2 · ALLOW_WOBBLE=0. Chosen against the canonical-core reading (level 3: 11 mined / 24 of 68 control records), level 4 (13 / 31), and not running the producer at all. It takes the plural reading of D3 and the most permissive non-vacuous setting on the grid — A4's own fail-closed direction for a sparing disjunct. Expected yield 3 mined of a 15-row / 12-locus ceiling; 8 of 68 control records lose (a)+(b). No ADR amended: A4 delegates the value to the round and the round records it in provenance; D6/D17's P6 freeze is untouched. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/tbox_finder/mining/architecture_param_recommend.py (2)
420-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the side-effecting comprehension with an explicit count.
seen.add(cid)returnsNone, so the condition works, but the guard depends on a side effect inside a comprehension filter. A direct count states the same rule without that dependency.♻️ Proposed refactor
- seen: set[str] = set() - dupes = sorted({cid for cid in manifest_ids if cid in seen or seen.add(cid)}) + counts = Counter(manifest_ids) + dupes = sorted(cid for cid, n in counts.items() if n > 1)Add
from collections import Counterto the imports.🤖 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_recommend.py` around lines 420 - 427, Replace the side-effecting `dupes` comprehension in the manifest validation block with an explicit `Counter`-based count of `manifest_ids`, selecting candidate IDs whose counts exceed one and preserving the existing sorted error behavior. Add the required `collections.Counter` import.
522-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind the ceiling id list once.
mined_ids(by_id, all_failed, stage2_threshold=None)runs twice for the two adjacent fields. Thestage2_not_declaredband entry already computed the same list. Bind it once so the published ids and the locus count cannot diverge.♻️ Proposed refactor
- "candidate_ids_at_the_ceiling": mined_ids(by_id, all_failed, stage2_threshold=None), - "distinct_loci_at_the_ceiling": len( - {locus_of(c) for c in mined_ids(by_id, all_failed, stage2_threshold=None)} - ), + "candidate_ids_at_the_ceiling": at_ceiling, + "distinct_loci_at_the_ceiling": len({locus_of(c) for c in at_ceiling}),Assign
at_ceiling = mined_ids(by_id, all_failed, stage2_threshold=None)before the return.🤖 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_recommend.py` around lines 522 - 527, In the return-building code around candidate_ids_at_the_ceiling and distinct_loci_at_the_ceiling, bind mined_ids(by_id, all_failed, stage2_threshold=None) once to at_ceiling before constructing the result, then reuse at_ceiling for both the published ID list and locus count; align with the existing stage2_not_declared computation so the same list is used consistently.tests/unit/test_architecture_param_recommend.py (1)
273-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the expected exception type.
pytest.raises(Exception, match="record|accession")passes on any error whose message contains those words, including aTypeErrororAttributeErrorraised before the contradiction check runs. The test then no longer proves that the delegated refusal is inherited. Assert the concrete error type thatload_control_recordsraises for a contradicting row.Run the following script to find the exception type the shipped loader raises:
#!/bin/bash # Description: Locate load_control_records and the error type it raises for a contradicting accession. fd -t f 'architecture_param_control_compare.py' -x ast-grep outline {} --items all rg -nP -C 12 'def load_control_records' --type=py🤖 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_recommend.py` around lines 273 - 277, Update test_control_records_refuses_a_manifest_row_that_contradicts_its_own_id to replace the broad pytest.raises(Exception) assertion with the concrete exception type raised by load_control_records for a contradictory accession, preserving the existing message match and delegated refusal scenario.
🤖 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_recommend.py`:
- Around line 809-814: Update the undecidable-axis calculation near
chosen.params.as_dict() to use a one-way set difference: include only parameter
keys absent from TIE_BREAK_VOCABULARY, rather than names exclusive to either
set. Keep the disagreeing comprehension’s comparisons unchanged and ensure every
key in undecidable is valid for both parameter dictionaries.
- Around line 137-143: Update the candidate_id validation in the surrounding
recommendation logic to require exactly four colon-separated fields, rejecting
both fewer and more than four fields with the existing RecommendError behavior.
Preserve the locus derivation for valid accession:contig:window:start-end
identifiers and prevent extra fields from being silently discarded by the return
expression.
---
Nitpick comments:
In `@src/tbox_finder/mining/architecture_param_recommend.py`:
- Around line 420-427: Replace the side-effecting `dupes` comprehension in the
manifest validation block with an explicit `Counter`-based count of
`manifest_ids`, selecting candidate IDs whose counts exceed one and preserving
the existing sorted error behavior. Add the required `collections.Counter`
import.
- Around line 522-527: In the return-building code around
candidate_ids_at_the_ceiling and distinct_loci_at_the_ceiling, bind
mined_ids(by_id, all_failed, stage2_threshold=None) once to at_ceiling before
constructing the result, then reuse at_ceiling for both the published ID list
and locus count; align with the existing stage2_not_declared computation so the
same list is used consistently.
In `@tests/unit/test_architecture_param_recommend.py`:
- Around line 273-277: Update
test_control_records_refuses_a_manifest_row_that_contradicts_its_own_id to
replace the broad pytest.raises(Exception) assertion with the concrete exception
type raised by load_control_records for a contradictory accession, preserving
the existing message match and delegated refusal scenario.
🪄 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: 6da9a8dc-ddd1-43bb-a1c2-68a2c0a750d3
⛔ Files ignored due to path filters (2)
analyses/phase3_log.qmdis excluded by!**/*.qmddata/processed/mining/round0_fp_spare_rule_inputs_v0.jsonis excluded by!data/**
📒 Files selected for processing (3)
reports/p3/architecture_parameter_recommendation.jsonsrc/tbox_finder/mining/architecture_param_recommend.pytests/unit/test_architecture_param_recommend.py
… the join, not on the next command CodeRabbit CLI, 1 major, real. load_spare_rule_inputs refuses a bool posterior and an out-of-range one; build_inputs — the command that WRITES the committed table — wrote float(post[cid]) and str(cov[cid]) with no checks. float(True) is 1.0, so a `true` in the source column would be committed as a value that is not a posterior, clear every threshold in the Stage-2 band, SPARE the candidate, and be invisible to the loader thereafter because by then the column really is a float. The guard ran after the artifact it guards existed. Both rules are now single functions — checked_status and checked_posterior — called by the writer AND the reader. The proposed patch would have added a second copy of each rule beside the existing one, which is how the writer came to have none of it. 6 tests added (110 total), 2 sabotages added (71 total, all RED against their named test + 2 green negative controls). The committed table re-derives to the identical sha256 (65aedc88…), so no current artifact changes. Zero new failures vs main at 090547f. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
…ce rather than a symmetric one Two CodeRabbit GitHub-app findings, both real. 1. locus_of refused FEWER than four colon-separated fields but accepted MORE. A five-field id would drop its middle fields and merge into another candidate's locus, shrinking every locus denominator with no refusal — the same failure control_records refuses for the record grouping. It now demands exactly four. 2. The tie refusal built its "axes TIE_BREAK_ORDER cannot speak to" with a SYMMETRIC difference. Today every vocabulary name is also a ParamTuple field so the two agree, but adding or renaming a non-parameter entry would put that name into `undecidable` and raise KeyError inside the guard that exists to prevent an undecided selection. Plain set difference now. 2 tests added (112), 2 sabotages added (73, all RED against their named test + 2 green negative controls). The report re-derives unchanged. Zero new failures vs main at 090547f. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
…, and the identity branch that had no test CodeRabbit CLI round 7 on a359e74: 2 minor findings, both real. (1) architecture_param_recommend read the FP manifest as manifest.get("candidates", manifest) at BOTH call sites, meaning to accept an object carrying a candidates list or the rows themselves. The second shape never worked: a top-level JSON list has no .get, and AttributeError is outside main's (RecommendError, ValueError, TypeError, OSError, KeyError), so the CLI ended in a traceback where a refusal was intended. An object WITHOUT candidates fell back to the mapping and iterated its keys. read_manifest_ids now serves both sites and refuses the shape, a non-object row (the producer's reader DROPS one, which here would silently shrink the 941 denominator behind the coverage check and the ceiling) and a row carrying neither id key (candidate_id_of raises ProducerError, a RuntimeError, which escapes the refusal path too). On the committed 941-row manifest the new reader returns the identical ids, so no published number moves. (2) floor_sensitivity's measured_identically_to_the_chosen_setting is three-valued and the test named for its True branch asserted False — its twin was strictly more permissive, so it won on the metric and the field could not be True. Renamed for what it asserts, and the True branch now has its own test, reached through the bulge floor (the one floor whose parameter is in the tie-break vocabulary). On the shipped grid that field takes only null and False, so the branch had no coverage at all. Inputs: reports/p3/architecture_parameter_recommendation.json, data/processed/mining/round0_fp_manifest.json (941 rows). Outputs: no artifact change — both reports re-derive unchanged; the manifest ids are byte-identical old vs new. Validation: 124 unit tests in the module (112 before); 8 sabotages this round, all RED against their NAMED test (81 cumulative); tests/unit + tests/golden vs the matched main baseline at 090547f — worktree 13 FAILED, main 26, a strict subset, zero new; ruff 0.15.15 + black 25.11.0 clean. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
…date the control's statuses, and three refusal-path escapes CodeRabbit CLI round 8 on fb6afcb: 5 findings (4 major, 1 minor), all real. (1) arms.comparison_headline_carried_forward copied a READING of the two arms out of the control-comparison report with no digest binding — the file's provenance hash identifies the file, not its subject, so a comparison of a different pair of arms could be quoted beside numbers derived from these two. assert_comparison_describes_these_supplies now checks arms.fp / arms.control against the digests measured here, and refuses an ABSENT digest as well as a disagreeing one. The shipped comparison records 90b2fe1b… / 0b5afbec…, equal to the two measurement reports', so the binding holds on the committed artifacts. This was on the step's "disclosed, not fixed" list; it is now fixed. (2) control_records validated the control's id set but never its status VALUES, while control_damage reads producibility as `status[q] in DECIDED_STATES` — so a typo, a null or a number silently left that query out of the 68-record denominator behind every share and interval on this arm. The FP arm's columns already pass checked_status in writer and reader. (3) assert_supply_is_the_decided_set compared set(supply_ids), so a supply naming one candidate twice merged into an exact-looking equality while every per-item loop still saw both rows. Ids are materialised and the repeat refused. (4) checked_posterior called float(value); a JSON integer beyond float range raises OverflowError, an ArithmeticError outside main's except tuple — a traceback where every other malformed posterior gets `refused:`. (5) A dead `if False` operand in the grid-distinctness assertion, whose unreachable branch read a `label` key as_dict() does not carry. Inputs: reports/p3/architecture_parameter_control_comparison.json (arms digests), reports/p3/architecture_parameter_measurement{,_control}.json. Outputs: no committed artifact changes. Validation: 135 unit tests in the module (124 before); 7 sabotages this round, all RED against their NAMED test, including two positive controls (a status guard that refuses everything; a grid whose points collide in value with the count preserved) — 88 cumulative; tests/unit + tests/golden vs the matched main baseline at 090547f — worktree 13 FAILED, main 26, zero new; ruff 0.15.15 + black 25.11.0 clean. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
…s file in its own refusal, and narrow an over-broad raises CodeRabbit CLI round 9 on 203bd90: 3 findings (1 major, 2 minor), all real. Two are on code round 8 wrote. (1) The slug -> candidate_id indexes were dict comprehensions for both arms, so two ids landing on one slug kept only the last: the consensus measured for that slug would be attributed to one candidate while the other left arch_by_id silently, every yield count still reconciling. The orphan check downstream only finds slugs with NO candidate, so it cannot see this. index_by_slug refuses it. Two distinct ids cannot collide by construction (candidate_slug appends a 12-hex digest of the exact id), so the guard states a contract and the test triggers the one reachable path — a repeated id. (2) Round 8's status-value refusal printed the MANIFEST path as the location of a bad STATUS value. control_records now takes status_path. The sabotage that mattered was the call site, not the helper: dropping the keyword-only argument left the suite green, so the pinning test asserts recommend's refusal names ctrl_status.json. (3) pytest.raises(Exception, ...) in an older test accepted AttributeError or KeyError as readily as the refusal it pinned; it names RecommendError / CompareError now. Outputs: no committed artifact changes. Validation: 139 unit tests in the module (135 before); 4 sabotages this round, all RED against their NAMED test (92 cumulative), including a positive control (an index refusing every id); tests/unit + tests/golden vs the matched main baseline at 090547f — worktree 13 FAILED, main 26, zero new; ruff 0.15.15 + black 25.11.0 clean. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
…ss found after the review gate came back clean CodeRabbit CLI round 10 returned "No new findings" on b521cdc. A 4-lens adversarial harness (27 agents, 0 errors, 23 candidates, 3 refuted by their own verifiers) then found 20 more, 17 of them in two families. FAMILY 1 — five more refusal-path escapes of the class round 7 closed for the manifest. `report.get("supply", {}).get(...)`, `comparison.get("headline", {}).get("reading")` and `(fp_report.get("helix_arm") or {}).get(...)` default only when the key is ABSENT, so a present-but-not-an-object value reaches `.get` and raises AttributeError, outside main's except tuple. The headline one is the sharpest: round 8 bound the comparison report's `arms` and left the field that guard exists to protect unchecked. read_json_object + object_field now serve all of them, and the control status table is shape-checked BEFORE index_by_slug consumes it — twelve lines ahead of control_records' own refusal. FAMILY 2 — nine published numbers no sabotage could reach. Each of these left all 139 tests green: `all(` -> `any(` on the record-level (a)AND(b) damage rule (the rule behind the 8-of-68 figure the §7 choice was made against); dropping the (a) conjunct from the query-level count; len(producible) -> len(queries) in the query denominator; each share divided by the other numerator; the two Wilson intervals swapped; frontier's argmin replaced by keep-the-last-seen; share_failed_of_decided divided by fp_passed; the locus collapse in mined_loci_by_threshold; the floors conjunct in dominating_alternatives. Cause: every control fixture was symmetric, so ALL was indistinguishable from ANY and (a)AND(b) from (b) alone. One asymmetric corpus now carries six quantities no two of which are equal; point()'s two record counts differ by default; the intervals are pinned BY IDENTITY (a containment assertion passed with them swapped, since at n=3 each interval contains the other's point estimate). Two guard call sites in recommend — the control decided-set check and the orphan check — were also deletable in silence and are now driven through recommend. Outputs: no committed artifact changes; the committed manifest still reads 941 ids through the new reader. Validation: 156 unit tests in the module (139 before); 13 sabotages this round, all RED against their NAMED test (105 cumulative); tests/unit + tests/golden vs the matched main baseline at 090547f — worktree 13 FAILED, main 26, zero new; ruff 0.15.15 + black 25.11.0 clean. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/unit/test_architecture_param_recommend.py`:
- Around line 378-379: Update the asymmetric_control fixture in
tests/unit/test_architecture_param_recommend.py so its published quantities are
all distinct: add one producible record with a passing (b) query or add a second
decided query to record C, then update the related expected counts and docstring
assertions at the other occurrences accordingly.
- Around line 1440-1444: Update
test_recommend_refuses_a_report_whose_supply_block_is_null to parametrize the
supply value with both null and the existing string shape, writing each value
into the report and asserting RecommendError for both cases. Keep the test name,
error match, and tiny_corpus setup aligned with the existing behavior.
🪄 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: 0b7a6dc1-b5a0-4457-81f0-a1ef7e5d86e6
⛔ Files ignored due to path filters (1)
analyses/phase3_log.qmdis excluded by!**/*.qmd
📒 Files selected for processing (2)
src/tbox_finder/mining/architecture_param_recommend.pytests/unit/test_architecture_param_recommend.py
Comment on lines
+1440
to
+1444
| def test_recommend_refuses_a_report_whose_supply_block_is_null(tiny_corpus): | ||
| """`.get("supply", {})` defaulted only for an ABSENT key; `null` reached `.get`.""" | ||
| Path(tiny_corpus["fp_report_path"]).write_text(json.dumps({"supply": "cluster"})) | ||
| with pytest.raises(rec.RecommendError, match="'supply' is"): | ||
| rec.recommend(**tiny_corpus) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The test writes a string, not null.
The test name and the docstring both name null as the trigger. The body writes {"supply": "cluster"}, so the JSON null case that reaches .get stays uncovered. Parametrize the value to cover both shapes.
💚 Proposed fix
-def test_recommend_refuses_a_report_whose_supply_block_is_null(tiny_corpus):
+@pytest.mark.parametrize(
+ "supply",
+ [pytest.param(None, id="null"), pytest.param("cluster", id="a_string")],
+)
+def test_recommend_refuses_a_report_whose_supply_block_is_not_an_object(tiny_corpus, supply):
"""`.get("supply", {})` defaulted only for an ABSENT key; `null` reached `.get`."""
- Path(tiny_corpus["fp_report_path"]).write_text(json.dumps({"supply": "cluster"}))
+ Path(tiny_corpus["fp_report_path"]).write_text(json.dumps({"supply": supply}))
with pytest.raises(rec.RecommendError, match="'supply' is"):
rec.recommend(**tiny_corpus)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_recommend_refuses_a_report_whose_supply_block_is_null(tiny_corpus): | |
| """`.get("supply", {})` defaulted only for an ABSENT key; `null` reached `.get`.""" | |
| Path(tiny_corpus["fp_report_path"]).write_text(json.dumps({"supply": "cluster"})) | |
| with pytest.raises(rec.RecommendError, match="'supply' is"): | |
| rec.recommend(**tiny_corpus) | |
| @pytest.mark.parametrize( | |
| "supply", | |
| [pytest.param(None, id="null"), pytest.param("cluster", id="a_string")], | |
| ) | |
| def test_recommend_refuses_a_report_whose_supply_block_is_not_an_object(tiny_corpus, supply): | |
| """`.get("supply", {})` defaulted only for an ABSENT key; `null` reached `.get`.""" | |
| Path(tiny_corpus["fp_report_path"]).write_text(json.dumps({"supply": supply})) | |
| with pytest.raises(rec.RecommendError, match="'supply' is"): | |
| rec.recommend(**tiny_corpus) |
🧰 Tools
🪛 ast-grep (0.45.1)
[info] 1441-1441: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"supply": "cluster"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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_recommend.py` around lines 1440 - 1444,
Update test_recommend_refuses_a_report_whose_supply_block_is_null to parametrize
the supply value with both null and the existing string shape, writing each
value into the report and asserting RecommendError for both cases. Keep the test
name, error match, and tiny_corpus setup aligned with the existing behavior.
…ic-control docstring CodeRabbit CLI round 11 on 280fd60: 1 minor, real. The fixture's summary line was corrected to "4 decided queries of 5" when the expectation was fixed, but the record-B bullet still read "(3) differs from the query total (4)". Four tests read this docstring as the primary statement of the expected counts. Validation: 156 unit tests green; ruff 0.15.15 + black 25.11.0 clean; no code change, no artifact change. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
… fires CodeRabbit CLI round 12 on 6b74067: 1 minor, real. A test named for a `null` supply block supplied the string "cluster". The two inputs take different branches: `null` is object_field's permissive case, so the DIGEST gate refuses it ("different supply"), not the shape gate ("'supply' is str"). The scalar case keeps the shape assertion under a matching name, and the null case is added under the guard that actually fires. Validation: 157 unit tests green (156 before); ruff 0.15.15 + black 25.11.0 clean; no code change, no artifact change. Claude-Session: https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
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′-f's block asked for its seven-parameter recommendation to be re-derived, not re-quoted, once the matched positive control landed.-g-ivlanded it. This is the re-derivation, over the whole admissible grid rather than the six named tuples the two measurement reports carry.The headline is not about (b): the round's yield ceiling is 15 of 941
Sparing is a disjunction (ADR-0005 D14), so mining is a conjunction. Criterion (a) fails on 111 of 941, criterion (c) on 96; their intersection is 15, and all 15 have a consensus so (b) is decidable on every one. Declaring the Stage-2 disjunct lowers it to 12 (thr 0.5) or 13 (0.9/0.95/0.99). Over the entire admissible grid the round mines between 1 and 13.
Criterion (b)'s seven parameters are therefore choosing among single-digit differences in a 941-candidate re-mining round. No committed report stated this, and it changes what the §7 question is about.
What the rule selects, and why it is a rule
Four floors are read off ADR-0006 D3's own wording and ADR-0004 D1's element vocabulary before any count is read —
min_named_helices ≥ 2,min_helix_pairs ≥ 2,ncca_pairing_nt ≥ 2, a boundedbulge_max_nt. Among the 864 survivors of 2,880 admissible points the rule takes the most permissive as measured, which is ADR-0006 A4's own fail-closed argument for a sparing disjunct. That selects exactlysensitive_core(1 · 2 · 2 · 2 · 50 · 2 · 0), tied only with its proved-inertallow_wobbletwin.Admissibility is measured by running the shipped localizer and catching its refusal, never by re-encoding its guard; each point's yield goes through the shipped
spare_rule.is_mining_excluded, not a re-derived conjunction.The composite (a) ∧ (b) is anti-selective at the strict end
-g-ivcompared (b)'s own failure share and found no separation. The object that decides what is mined is the conjunction, and there the control falls through more often than the FP arm at five of six named settings — 9.2 vs 7.2 %, 10.5 vs 11.5 %, 38.2 vs 23.7 %, 43.4 vs 32.0 %, 57.9 vs 37.4 %, 59.2 vs 38.5 % — with disjoint 95 % Wilson intervals at the two strictest. Criterion (a) passes 39.5 % of the control against 60.1 % of the FP arm, so tightening (b) removes protection from a population (a) was already protecting less.Disclosures carried in the artifact
TODO.mdandimp.mdrecord; the committedsynteny_exclusion_diagnostic.jsonsays so.slurm/p3/stage1_remine.sbatchbuilds--relaxed-arch-available/--synteny-availableinto$PLAN_FLAGSbut never passes--relaxed-arch-statusor--synteny-status, andremine.py:512refuses exactly that pair. Reported, not patched — off this step's roadmap.Validation
Zero new failures vs a matched
mainbaseline at090547f(worktree 13 FAILED, main 26, a strict subset; the delta is the documented DVC/LFS skips, each named). 40 sabotages all RED against their NAMED test + 2 green negative controls; the harness rewrites the original bytes (the module is untracked, sogit checkout --would be a silent no-op), clears__pycache__, and re-readsgit status --porcelainaround every mutation. Two defects of this step were caught by that harness rather than by review: a duplicated floor filter that letclears_all_floorsbe neutered with no effect on the selection, and ameasured_identicallyclause that was vacuously TRUE whenever the selection had not moved. Both reports re-derive byte-identically apart fromgenerated_at_utc.ruff+blackclean.pins_nothing: true; no ADR amended — A4 already delegates the value to the round. The seven-parameter §7 choice is the project lead's and this PR does not take it.https://claude.ai/code/session_019FP5o18yYaQvzejBfkjwAB
Summary by CodeRabbit