P3-15′-b: the per-candidate Stage-2 posterior producer + the supply derivation - #111
Merged
Merged
Conversation
… supply derivation
The mining spare rule's fourth disjunct (ADR-0005 D14) has had a pinned predicate
since P2-07 and no backend: the round-0 FP manifest carries coordinates, and
remine.load_stage2_posteriors wants candidate_id -> posterior. This is the leg
between them, and the mirror of covariation_producer for the covariation-(a) disjunct.
Inputs: data/processed/mining/round0_fp_manifest.json (941, git-tracked)
data/interim/production_genomes/*.fna (2,500, DVC)
P3-06 production arm aux1.0_lr1e-4; reports/gate2_p3_ece.json (T read, not re-typed)
Outputs: $ROUND_DIR/stage2_posteriors.json (wrapper form the consumer unwraps)
reports/p3/stage2_producer_control.json (the committed designed control)
remine.STAGE2_SUPPLY_AVAILABLE = True + a two-way flag pair + an exit-4 preflight
Composes, re-derives nothing: resolve_candidate_sequence -> transcribe_to_rna ->
score_rows -> calibrated_posterior, the calibrated array read through
payload[payload["gated_posterior_key"]] so the D11 indirection has one implementation.
No priors are passed, so what is published is the D11 named (PRE-prior-shift) object.
The span is the bare locus: stage2/dataset.py raises on a non-zero flank, so the head
was trained and its temperature fitted on bare loci and there is deliberately no flank
knob. An unresolvable or unscored candidate is OMITTED, never written as 0.0 — absent
resolves to unavailable => spared, a literal zero to failed => mined.
Strand is a §7 fork resolved on measurement (bioedca, 2026-08-05: max_over_strands).
The manifest carries no strand and Stage-1 is RC-equivariant. Scoring both strands of
all 941: the minus strand is higher on 458/941, and at a 0.9 operating point 119
candidates (12.6%) pass ONLY on the minus strand (+ = 7e-6 vs - = 0.9997). A plus-only
reading returns `failed` on those — a satisfied mining conjunct on loci Stage-2 calls
near-certain T-boxes. --strand-policy is required with no default; both per-strand
posteriors ride in the report; posterior_kind states that a max of two draws is a
SELECTION on the object GATE-2 graded.
The merge carries the denominator the consumer does not: load_stage2_posteriors accepts
{} silently and [ -s ] passes on a two-byte {}, so a truncated table would certify as a
clean round of nothing. --min-coverage defaults to 1.0 and refuses below it.
derive_stage2_supply_available() re-derives the constant from six named, independently
breakable, fail-closed clauses over git-tracked, non-DVC evidence only — so CI, laptop
and cluster answer identically. The unit pin asserts agreement, not a literal True, so
the constant cannot drift in either direction.
Measured (laptop RTX 4060, all 941): coverage 1.0000, 0 unresolved; 2.0s resolve +
18.9s load + 38.7s scoring (1,882 rows, 0.0206 s/row); 1.33 GiB peak VRAM. Control:
positive 0.999420, shuffle 0.000036, margin 0.999384 vs floors 0.90/0.10/0.50 fixed
before the measurement.
This does NOT move the yield: mining is a conjunction and relaxed_architecture +
downstream_aaRS_synteny still have no backend, so may_run stays False with max_mined 0.
A change that makes those gates agree is a regression, and the test says so.
Validation: 58 unit + 8 ml-tier tests; full unit+golden+ml 4,818 passed / 14 failed vs a
matched baseline worktree at 028e508 (4,755 / 14) — failure sets identical, comm empty
both ways. 39 sabotages, 38 RED against their named test, the 39th the declared no-op
control; a 40th targeting the torch-only tier verified by execution against the real
checkpoint. ruff 0.15.15 + black 25.11.0 clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…v has no editable install Measured on `two`, not assumed: inside tbox-ml-rna at ~/tbox-finder, `python -c "import tbox_finder"` is a ModuleNotFoundError, so all three `python -m tbox_finder.mining.stage2_producer` calls and the inline consumer-readback heredoc would have died on the first line of real work. Every other sbatch in the repo already prefixes `PYTHONPATH=src` for exactly this reason. `sbatch --test-only` cannot catch this — it validates partition/gres/time and nothing about the body — so the preflight was green while the job was not runnable. Exported once after the `cd "$REPO"` so it covers the heredoc leg too. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
|
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 a Stage-2 posterior producer with strand-aware scoring, calibration, controls, shard merging, and supply verification. It adds SLURM execution, committed control evidence, remining CLI enforcement, and CPU/ML test coverage. ChangesStage-2 posterior supply
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SLURMArray as stage2_producer.sbatch
participant ProducerCLI as stage2_producer CLI
participant PosteriorTables as posterior tables
participant ConsumerLoader as consumer loader
SLURMArray->>ProducerCLI: create deterministic manifest shard
SLURMArray->>ProducerCLI: score shard with strand policy and CUDA
ProducerCLI->>PosteriorTables: write validated shard table
SLURMArray->>ProducerCLI: merge complete shard tables
ProducerCLI->>PosteriorTables: write merged posterior table
SLURMArray->>ConsumerLoader: load merged posterior table
ConsumerLoader-->>SLURMArray: return validated posterior map
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
All nine reproduced by execution before fixing. The three majors were holes in the
guards themselves.
MAJOR - the supply derivation could return available:True with a clause MISSING.
`production_arm_on_record` is set in the `except` branch or in the
`if fingerprint is not None and production is not None` branch; if
`production_arm_config()` returns None while `sweep_fingerprint` succeeds, NEITHER runs,
the key is absent, and `all(clauses.values())` skips it — a clause that disappears reads
as a clause that passed. Reproduced: 5 of 6 clauses reporting, available=True. This is
the exact emptiness hole the module already documents for REQUIRED_CONTROL_FLAGS, one
level up. SUPPLY_CLAUSES now names the set and an unreported clause is filled with a
refusal. The unit pin keeps its OWN literal tuple — asserting against SUPPLY_CLAUSES
would be a tautology now that the function fills from it.
MAJOR - two array jobs sharing a ROUND_DIR both published to it. Markers, shard tables
and the merge flock are all per-ARRAY_ID, but the published stage2_posteriors.json is
not, so both jobs pass their own completeness test and the second silently replaces the
first — possibly with a different strand policy. An atomic noclobber ownership claim now
runs before any GPU time, and was moved above `conda activate` so it fails fast and is
testable off-cluster.
MAJOR - N_SHARDS=3 with --array=0-1 passed the per-task bound check: both tasks succeed,
N_DONE stays 2, the completeness test never fires and NOBODY merges. The full array shape
(MIN/MAX/COUNT) is now validated, with a local-execution fallback.
MINOR - ROUND_DIR was pattern-matched, so `round-output`, `$REPO` itself, and anything
reachable via `..` or a symlink were accepted and wrote into the repo after the cd. Both
paths are canonicalised with `realpath -m` and a relative value is refused.
MINOR - PYTHONPATH put an inherited value FIRST, so another checkout on the environment
would have shadowed the revision the job was submitted from.
MINOR - clause 4 imported the consumer unguarded, so a broken `remine` propagated out of
a function documented fail-closed on every clause.
MINOR - the call-time-resolution test could not show what it claimed: with both sides
True, a definition-time default satisfies `plan[...] is STAGE2_SUPPLY_AVAILABLE`. The
constant is now CHANGED on the module object (the file's `from` import is a separate
binding a monkeypatch does not reach) and the plan must follow.
MINOR - `sorted(..., reverse=True)` bound plus/minus BACKWARDS (ord('+')=43 < ord('-')=45);
symmetric assertions hid it, and any asymmetric one added later would have tested the
other strand. Selected by value now.
MINOR - the gate2 fixture's nested merge dropped every sibling calibration key
(beta/calib_prevalence/fitted_on), so two parametrized clause cases were failing partly
for a reason other than the one under test — the opposite of the one-mutation-at-a-time
discipline the helper exists to provide.
New guards executed, not assumed: relative/$REPO/.. ROUND_DIR refused, matched shape
accepted; short and offset arrays refused, matched accepted; a foreign owner refused, a
job re-entering its own round dir accepted.
Validation: 115 tests in the two amended files; full unit+golden+ml 4,821 passed /
14 failed vs the matched baseline at 028e508 (4,755 / 14) — failure sets identical, comm
empty both ways. 39 sabotages, 39 behaved as declared. ruff + black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
MAJOR - the non-blocking merge lock could leave NOBODY merging. `flock -n` lets a task that loses the race exit without merging while the lock holder is still counting a short marker set. Reproduced by execution: task A takes the lock and counts 1 of 2 (B has not written its marker yet); B writes its marker, fails the lock and exits; A then exits on its stale count. Both markers exist, no merge ever ran — the array pays in full and publishes nothing. The lock now BLOCKS, so the second task counts the complete set, and MERGE_OK is re-checked after acquisition so waiting cannot become a double merge. Verified both ways on a harness reproducing the interleaving: nonblocking => "nobody merged", blocking => merged. MINOR - a control record carrying booleans certified. `isinstance(True, int)` is True, so "positive_posterior": true / "shuffle_posterior": false clears both floors AND the margin (1.0 - 0.0) — a perfect separation made of no measurement. `validate_posteriors` already excluded bool by name; the control clause did not, so two readers of the same quantity disagreed about what counts as a number. Both now use one `_is_real_number`. MINOR - a non-mapping `posteriors` payload raised AttributeError/TypeError instead of the named refusal. `_cmd_merge` catches only Stage2ProducerError, so a list/str/null table produced a traceback and a generic exit code rather than the FATAL message and exit 3 the sbatch branches on. Tested through the CLI, since the exit code is what the sbatch reads. MINOR - a test comment named four clauses while asserting three. Validation: 121 tests in the two amended files; full unit+golden+ml 4,827 passed / 14 failed vs the matched baseline at 028e508 (4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39 behaved as declared. ruff + black clean. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…d real SHAs Records the two CLI rounds (13 findings, all real, 4 major), the unmet third round, and the preflight escape: `sbatch --test-only` was green on a job that could not run, so it is not evidence that a job will start doing work. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…producibility measurement Job 1163 (--array=0-1, one A4000/task) completed in ~5 min. Artifact-based verification per §9.3 step 8: squeue terminal, both SHARD_OK markers plus MERGE_OK, .owner reads 1163, 941/941 at coverage 1.0000 with 0 unresolved, and the in-job readback through remine.load_stage2_posteriors accepted 941 posteriors in [3.99e-06, 0.999673]. The two .err files carry no traceback. The merge leg exercised the round-2 fix exactly: task 0 counted 1/2 under the blocking lock and deferred, task 1 counted the complete set and merged — the interleaving the pre-fix `flock -n` turned into "nobody merges". MEASURED and recorded: the produced posterior is not bit-reproducible across device + sharding. Cluster (A4000, 2 shards) vs laptop (4060, 4 shards): 0/941 identical, median |Δ| 1.25e-06, max 3.79e-02, 12 candidates over 1e-2. Verdict flips are 0 at 0.5/0.7/0.9/ 0.95/0.999 and 3 at 0.99 — all three sitting ON the threshold (Δ ~ 3.3e-04), all in the spared-on-laptop / mined-on-cluster direction. Carried to the §13.1 gate because it says where the unpinned STAGE2_THRESHOLD may sensibly go: 0.99 decides 3 of 941 by numerics, 0.9 decides the same 236 on either machine with a wide margin. The table itself is NOT committed — $ROUND_DIR is scratch by design. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
MAJOR - the merge coerced BEFORE validating, which laundered exactly the types
`validate_posteriors` rejects by name. Reproduced: a shard carrying "posteriors": {"c0":
true} merged as 1.0 and certified; "0.5" merged as 0.5; and null raised a bare TypeError,
which `_cmd_merge` does not catch, so the operator got a traceback and a generic exit code
instead of the named refusal and exit 3 the sbatch branches on. The raw value is validated
first, through the same `_is_real_number` the control clause uses.
MINOR - clause 2's docstring claimed more than it enforced. `production_arm_config()`
returns only (aux_weight, lr), so comparing that pair against the report of the arm GATE-2
names is nearly a tautology: it cannot notice that conf/ would select a DIFFERENT arm than
the one graded. `arms_matching_config` now resolves the arm NAME from the git-tracked
sweep reports (not `discover_arms`, which needs the DVC checkpoint root and is False-in-CI)
and the clause requires it to equal the graded arm.
MINOR - the ml tier resolved default Stage-2 paths from the CWD.
⚠ The third finding was RIGHT about the sweep reports and WRONG about the checkpoint, and
the two must be split ([[review-finding-right-fix-wrong]], fifth occurrence in this repo).
The sweep reports are git-tracked, so anchoring them on the test file's repo is what makes
them CWD-independent. The checkpoints are DVC-tracked: they are materialised in the main
checkout and absent from a linked worktree — which eval.discover_arms records in its own
comment. Anchoring those on _REPO turned a green gate into four hard failures, measured.
The checkpoint root is now TBOX_STAGE2_CKPT_ROOT-overridable and defaults to the
CWD-relative path the run actually uses; the sweep dir keeps the repo anchor.
⚠ One test had to be re-aimed rather than kept: the clause-backstop test drove the missing
-clause path through `production_arm_config() -> None`, and the round-3 clause-2 work
closed that route at its source. A test aimed at a path that can no longer be reached
measures nothing, so it now exercises the backstop directly (a clause name the body never
sets) and a separate test covers the now-refusing route.
Validation: 128 tests in the two unit files, ml tier 8/8 armed on the real checkpoint;
full unit+golden+ml 4,834 passed / 14 failed vs the matched baseline at 028e508
(4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39 behaved as
declared. ruff + black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
A duplicate candidate_id INSIDE one shard was undetected. `merge_posterior_tables` refuses a candidate claimed by two shard tables but cannot see a repeat within one: both rows get the same row_id, `score_rows` keys its output on str(row_id), and the two resolve to the last-scored logit — with len(scored) == len(rows) still holding, so the shear is silent ([[duplicate-key-merges-instead-of-colliding]]). There was a downstream backstop — the collapsed pair drops n_scored, so coverage falls below the floor and the merge refuses — but it names the wrong cause, and only while --min-coverage is 1.0. `build_rows` now refuses the duplicate by name, with a positive control asserting two DISTINCT candidates over the same span are still fine (the guard keys on the id, not the coordinates). Validation: full unit+golden+ml 4,835 passed / 14 failed vs the matched baseline at 028e508 (4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39 behaved as declared. ruff + black clean. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
MINOR - `strand_posteriors` and `unresolved` got no named refusal. `posteriors` was
guarded at round 2; its two siblings were not, so a shard carrying
"strand_posteriors": [...] raised AttributeError on .items() and "unresolved": {...} (or
a list of strings) raised ValueError in dict(u). Neither is a Stage2ProducerError, so
_cmd_merge let them out as a traceback with a generic exit code instead of exit 3 — the
third instance of this same shape in this file (after the non-mapping posteriors payload
and the coerce-before-validate defect), now closed on every field the merge reads.
MINOR - an ml-tier test claimed a precondition its body did not enforce. It never called
_need_stack(), and the derivation reads only git-tracked evidence, so it ran identically
in CI and merely duplicated two unit tests. It now requires the stack AND asserts the
thing only this tier can: that the arm the loader would OPEN is the arm the git-tracked
sweep evidence names — the one check that ties the DVC-tracked bytes to the committed
record on the same box.
Validation: full unit+golden+ml 4,838 passed / 14 failed vs the matched baseline at
028e508 (4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39
behaved as declared. Armed ml tier 8/8 on the real checkpoint. ruff + black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
MINOR - `run_control` indexed named[0]/named[1] without the length guard `score_to_posteriors` applies before its own join. A short `score_rows` return raised IndexError, which `_cmd_control` does not convert into the named refusal, so the operator got a traceback instead of a control that failed. Guarded and covered by a stub-scorer test on the bare CI path. MINOR - a unit test resolved `sweep_fingerprint`'s sweep_dir from the CWD; anchored on SP.REPO_ROOT (the sweep reports are git-tracked, so the repo anchor is the correct one here — unlike the DVC-tracked checkpoint root, which must stay CWD/env-resolved). Validation: full unit+golden+ml 4,839 passed / 14 failed vs the matched baseline at 028e508 (4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39 behaved as declared. ruff + black clean. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
![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
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/remine.py (1)
770-793: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle the
may_runoverride inremine_problems.
When both recorded gates are true, the forcedmay_run: falsedisagrees with the derived value and adds a spurious self-check problem to the artifact.🤖 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/remine.py` around lines 770 - 793, Update remine_problems to recognize the supply_declaration_unevidenced may_run override: when both recorded gates are true and the report identifies this override, do not report a self-check mismatch for the forced may_run: false value. Preserve the existing validation for reports without this override.
🧹 Nitpick comments (5)
src/tbox_finder/mining/remine.py (1)
662-675: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_supply_derivationsignores itsargsparameter.Both derivations take no input from
args. The parameter suggests a per-run dependency that does not exist, and every call site passes a namespace only to satisfy the signature. Drop the parameter.♻️ Proposed simplification
-def _supply_derivations(args: argparse.Namespace) -> dict[str, Any]: +def _supply_derivations() -> dict[str, Any]:Update the two call sites accordingly:
derivations = _supply_derivations() ... unevidenced = _refuse_unevidenced(args, _supply_derivations())🤖 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/remine.py` around lines 662 - 675, Remove the unused args parameter from _supply_derivations and update every call site to invoke it without arguments, including the derivations assignment and the _refuse_unevidenced call. Preserve both existing derivation results and the surrounding flow.tests/unit/test_mining_spare_rule.py (1)
345-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe blocking-disjunct assertion depends on list order.
test_the_measured_p2_state_is_ready_and_yields_nothingat Line 281 sorts before comparing. This assertion compares the raw list, so it also pins the iteration order ofMODEL_INDEPENDENT_DISJUNCTS. A reorder of that constant breaks this test for a reason unrelated to supply. Sort here as well, or state in a comment that the order is intentional.♻️ Proposed change
- assert plan["yield"]["blocking_disjuncts"] == [ - "relaxed_architecture", + assert sorted(plan["yield"]["blocking_disjuncts"]) == [ "downstream_aaRS_synteny", + "relaxed_architecture", ]🤖 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_mining_spare_rule.py` around lines 345 - 348, Update the blocking_disjuncts assertion in test_the_measured_p2_state_is_ready_and_yields_nothing to compare sorted values, avoiding dependence on MODEL_INDEPENDENT_DISJUNCTS iteration order while preserving the expected disjunct set.tests/unit/test_stage2_producer.py (1)
335-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo supply-derivation tests sit under the merge banner.
test_the_config_and_the_graded_arm_must_name_the_SAME_checkpointandtest_a_config_naming_a_different_arm_than_gate2_fails_clause_2test clause 2 ofderive_stage2_supply_available. They are placed between the merge tests, under the "merge — the denominator the consumer does not carry" banner.test_a_control_certified_on_BOOLEANS_does_not_passat Line 486 has the same problem. Move all three under the "The supply derivation" banner at Line 678 so the section headers stay accurate.🤖 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_stage2_producer.py` around lines 335 - 374, Move test_the_config_and_the_graded_arm_must_name_the_SAME_checkpoint, test_a_config_naming_a_different_arm_than_gate2_fails_clause_2, and test_a_control_certified_on_BOOLEANS_does_not_pass from the merge-test section to the “The supply derivation” section, preserving their implementations and order relative to the surrounding supply-derivation tests.src/tbox_finder/mining/stage2_producer.py (2)
465-586: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCross-shard
strand_posteriorscollisions are not refused.
mergedrefuses acandidate_idclaimed by two shards.strand_posteriors[cid] = dict(per)does not. A shard that carries astrand_posteriorsentry for a candidate it did not score silently overwrites another shard's per-strand record, and the audit column then disagrees with the published posterior. The published number is unaffected, so this is an auditability gap, not a scoring gap.♻️ Proposed refusal, mirroring the posteriors guard
for cid, per in shard_strands.items(): if not isinstance(per, Mapping): raise Stage2ProducerError( f"{path}: strand_posteriors[{cid!r}] is {type(per).__name__}, not a " "strand → posterior mapping" ) + if cid in strand_posteriors: + raise Stage2ProducerError( + f"{path}: strand_posteriors for {cid!r} appears in more than one shard " + "table — the audit record would silently lose one of the two" + ) strand_posteriors[cid] = dict(per)🤖 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/stage2_producer.py` around lines 465 - 586, Update merge_posterior_tables so each strand_posteriors candidate_id is rejected when already present in strand_posteriors, and when it is not present in merged. Raise Stage2ProducerError with the shard and candidate context before assigning the entry, preserving the existing cross-shard collision refusal behavior used for posterior rows.
1114-1131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
score-sharddoes not convert a named refusal into a documented exit code.
_cmd_mergecatchesStage2ProducerErrorand returns 3._cmd_score_sharddoes not.build_rowsraisesStage2ProducerErroron a duplicatecandidate_idin a shard, andvalidate_posteriorsraises it insidebuild_table. The operator then gets a traceback and exit 1 instead of the named refusal. The sbatch stops on any non-zero code, so the run is still fail-closed; only the diagnosis suffers._cmd_controlhas the same gap forread_control_positive.♻️ Proposed alignment with `_cmd_merge`
def _cmd_score_shard(args: argparse.Namespace) -> int: specs = read_candidate_manifest(args.shard) - table = score_shard( - specs, - temperature=read_temperature(args.gate2_report), - strand_policy=args.strand_policy, - genome_dir=args.genome_dir, - checkpoint_root=args.checkpoint_root, - sweep_dir=args.sweep_dir, - batch_size=args.batch_size, - device=args.device, - ) + try: + table = score_shard( + specs, + temperature=read_temperature(args.gate2_report), + strand_policy=args.strand_policy, + genome_dir=args.genome_dir, + checkpoint_root=args.checkpoint_root, + sweep_dir=args.sweep_dir, + batch_size=args.batch_size, + device=args.device, + ) + except Stage2ProducerError as exc: + print(f"FATAL: {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/stage2_producer.py` around lines 1114 - 1131, Update _cmd_score_shard and _cmd_control to catch Stage2ProducerError around their existing processing calls and return the documented refusal exit code 3, matching _cmd_merge. Ensure duplicate candidate IDs, posterior validation failures, and read_control_positive errors produce the named refusal without an uncaught traceback.
🤖 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 `@slurm/p3/stage2_producer.sbatch`:
- Around line 171-180: Update the ownership-claim logic around OWNER_FILE so a
task that loses the noclobber create race retries reading the owner for a
bounded interval, allowing the winning writer time to persist ARRAY_ID. After
retries are exhausted, treat an empty or unreadable owner as a refusal and
retain the existing mismatch failure behavior; only continue when the persisted
owner matches ARRAY_ID.
---
Outside diff comments:
In `@src/tbox_finder/mining/remine.py`:
- Around line 770-793: Update remine_problems to recognize the
supply_declaration_unevidenced may_run override: when both recorded gates are
true and the report identifies this override, do not report a self-check
mismatch for the forced may_run: false value. Preserve the existing validation
for reports without this override.
---
Nitpick comments:
In `@src/tbox_finder/mining/remine.py`:
- Around line 662-675: Remove the unused args parameter from _supply_derivations
and update every call site to invoke it without arguments, including the
derivations assignment and the _refuse_unevidenced call. Preserve both existing
derivation results and the surrounding flow.
In `@src/tbox_finder/mining/stage2_producer.py`:
- Around line 465-586: Update merge_posterior_tables so each strand_posteriors
candidate_id is rejected when already present in strand_posteriors, and when it
is not present in merged. Raise Stage2ProducerError with the shard and candidate
context before assigning the entry, preserving the existing cross-shard
collision refusal behavior used for posterior rows.
- Around line 1114-1131: Update _cmd_score_shard and _cmd_control to catch
Stage2ProducerError around their existing processing calls and return the
documented refusal exit code 3, matching _cmd_merge. Ensure duplicate candidate
IDs, posterior validation failures, and read_control_positive errors produce the
named refusal without an uncaught traceback.
In `@tests/unit/test_mining_spare_rule.py`:
- Around line 345-348: Update the blocking_disjuncts assertion in
test_the_measured_p2_state_is_ready_and_yields_nothing to compare sorted values,
avoiding dependence on MODEL_INDEPENDENT_DISJUNCTS iteration order while
preserving the expected disjunct set.
In `@tests/unit/test_stage2_producer.py`:
- Around line 335-374: Move
test_the_config_and_the_graded_arm_must_name_the_SAME_checkpoint,
test_a_config_naming_a_different_arm_than_gate2_fails_clause_2, and
test_a_control_certified_on_BOOLEANS_does_not_pass from the merge-test section
to the “The supply derivation” section, preserving their implementations and
order relative to the surrounding supply-derivation tests.
🪄 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: fac8c305-4aca-4974-ba77-3ea35a4b7df7
⛔ Files ignored due to path filters (1)
analyses/phase3_log.qmdis excluded by!**/*.qmd
📒 Files selected for processing (8)
.gitignorereports/p3/stage2_producer_control.jsonslurm/p3/stage2_producer.sbatchsrc/tbox_finder/mining/remine.pysrc/tbox_finder/mining/stage2_producer.pytests/ml/test_stage2_producer_control.pytests/unit/test_mining_spare_rule.pytests/unit/test_stage2_producer.py
Comment on lines
+171
to
+180
| OWNER_FILE="$ROUND_DIR/.owner" | ||
| if ! ( set -o noclobber; echo "$ARRAY_ID" > "$OWNER_FILE" ) 2>/dev/null; then | ||
| OWNER="$(cat "$OWNER_FILE" 2>/dev/null || true)" | ||
| if [ "$OWNER" != "$ARRAY_ID" ]; then | ||
| echo "FATAL: $ROUND_DIR is already owned by array job ${OWNER:-<unreadable>}," >&2 | ||
| echo " and this is array job $ARRAY_ID. Two jobs publishing one round table" >&2 | ||
| echo " would overwrite each other. Submit with a fresh ROUND_DIR." >&2 | ||
| exit 2 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The ownership claim has a read-before-write race that can kill a sibling task.
set -o noclobber; echo "$ARRAY_ID" > "$OWNER_FILE" performs two steps: an O_EXCL create, then a write. Both array tasks of the same job start within milliseconds of each other. If task 1 loses the create race and reads $OWNER_FILE before task 0 has written its bytes, OWNER is the empty string. The comparison [ "$OWNER" != "$ARRAY_ID" ] is then true, and task 1 exits 2 with owned by array job <unreadable>.
The consequence is worse than one lost task. Task 1's shard is never scored, its SHARD_OK marker is never written, N_DONE never reaches N_SHARDS, and no task merges. The array reports a partial success and publishes nothing, which is the silent no-round the header warns about at Line 140.
Retry the read for a bounded interval, and treat a persistently empty owner file as the refusal.
🐛 Proposed fix for the read-before-write race
OWNER_FILE="$ROUND_DIR/.owner"
if ! ( set -o noclobber; echo "$ARRAY_ID" > "$OWNER_FILE" ) 2>/dev/null; then
- OWNER="$(cat "$OWNER_FILE" 2>/dev/null || true)"
+ # The winning task creates the file with O_EXCL and writes to it as a second step,
+ # so a loser can observe it empty. Poll instead of reading once.
+ OWNER=""
+ for _ in $(seq 1 50); do
+ OWNER="$(cat "$OWNER_FILE" 2>/dev/null || true)"
+ [ -n "$OWNER" ] && break
+ sleep 0.2
+ done
if [ "$OWNER" != "$ARRAY_ID" ]; then
echo "FATAL: $ROUND_DIR is already owned by array job ${OWNER:-<unreadable>}," >&2📝 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
| OWNER_FILE="$ROUND_DIR/.owner" | |
| if ! ( set -o noclobber; echo "$ARRAY_ID" > "$OWNER_FILE" ) 2>/dev/null; then | |
| OWNER="$(cat "$OWNER_FILE" 2>/dev/null || true)" | |
| if [ "$OWNER" != "$ARRAY_ID" ]; then | |
| echo "FATAL: $ROUND_DIR is already owned by array job ${OWNER:-<unreadable>}," >&2 | |
| echo " and this is array job $ARRAY_ID. Two jobs publishing one round table" >&2 | |
| echo " would overwrite each other. Submit with a fresh ROUND_DIR." >&2 | |
| exit 2 | |
| fi | |
| fi | |
| OWNER_FILE="$ROUND_DIR/.owner" | |
| if ! ( set -o noclobber; echo "$ARRAY_ID" > "$OWNER_FILE" ) 2>/dev/null; then | |
| # The winning task creates the file with O_EXCL and writes to it as a second step, | |
| # so a loser can observe it empty. Poll instead of reading once. | |
| OWNER="" | |
| for _ in $(seq 1 50); do | |
| OWNER="$(cat "$OWNER_FILE" 2>/dev/null || true)" | |
| [ -n "$OWNER" ] && break | |
| sleep 0.2 | |
| done | |
| if [ "$OWNER" != "$ARRAY_ID" ]; then | |
| echo "FATAL: $ROUND_DIR is already owned by array job ${OWNER:-<unreadable>}," >&2 | |
| echo " and this is array job $ARRAY_ID. Two jobs publishing one round table" >&2 | |
| echo " would overwrite each other. Submit with a fresh ROUND_DIR." >&2 | |
| exit 2 | |
| fi | |
| fi |
🤖 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 `@slurm/p3/stage2_producer.sbatch` around lines 171 - 180, Update the
ownership-claim logic around OWNER_FILE so a task that loses the noclobber
create race retries reading the owner for a bounded interval, allowing the
winning writer time to persist ARRAY_ID. After retries are exhausted, treat an
empty or unreadable owner as a refusal and retain the existing mismatch failure
behavior; only continue when the persisted owner matches ARRAY_ID.
MINOR - `run_control` called `load_record.get(...)` unguarded while `build_table` types
the same record as optional, so a None record raised AttributeError — which `_cmd_control`
turns into a traceback rather than a control that failed.
Guarded, and taken one step further than the proposed `(load_record or {}).get(...)`:
writing the hashes as nulls only defers the failure to
`control_matches_this_calibration`, which reads exactly those two fields, and reports it
there with a vaguer cause. A control that cannot name the bytes it was earned against is
not evidence for any checkpoint, so it is refused by name at the point of production.
Validation: full unit+golden+ml 4,843 passed / 14 failed vs the matched baseline at
028e508 (4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39
behaved as declared. Armed ml tier 8/8 on the real checkpoint. ruff + black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
MINOR - a cross-shard `strand_posteriors` duplicate was silently overwritten while the `posteriors` loop refused one. The per-strand values are the AUDIT TRAIL for the emitted posterior — they are what makes `emitted == max(+, -)` checkable — so a silent replacement leaves the published number unverifiable. Refused by name. MINOR - the "no src module hardcodes the fitted temperature" pin could become non-specific. The needle is the first 10 chars of repr(T); the existing guard only excluded exactly 1.0, so a re-fit to a short value like 1.5 would search src/ for "1.5" and report unrelated files, failing the pin for a reason unrelated to a hardcoded temperature. A minimum needle length is now asserted with a message naming the real cause. Validation: full unit+golden+ml 4,844 passed / 14 failed vs the matched baseline at 028e508 (4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39 behaved as declared. ruff + black clean. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
MAJOR - the control payloads were never checked against the context window. `build_rows` asserts it because `score_rows` calls the tokenizer's bare `encode`, which does not enforce it — an over-length payload is silently truncated into a different sequence than the one named. `run_control` scores a tbdb FASTA_sequence of unbounded length and hashes the FULL sequence into the record, so an over-length positive would certify against bytes the model never scored while `control_matches_this_calibration` still passed. Both control payloads now carry the same assertion. MAJOR - the merge never checked that a merged id belongs to THIS round. Given only a count, a stale or foreign shard table passed in --tables contributes ids outside the manifest, len(merged) rises, the coverage floor passes — and the truncated-run refusal this function exists to make is defeated by exactly the argument its own docstring makes. `manifest_ids` now refuses an unknown id by name (the CLI already read the manifest for the count, so it passes the ids for free), with a count-only backstop for callers that cannot supply them. MINOR - `--min-coverage` accepted any float, so `--min-coverage 0` disabled the floor entirely and published a table that scored 3 of 941 as a clean round. Clamped to (0, 1]. MINOR - a positive-control test asserted `rc != 4`, which almost any outcome satisfies — including the monkeypatch missing its target and the real derivation running. It asserts the exact code the round refuses with. MINOR - `_stage_evidence` wrote the control report without creating its parent, succeeding only because the sweep mkdir happens to create the same directory; moving CONTROL_REPORT would have failed every test routed through the helper with FileNotFoundError instead of exercising the clause it names. Validation: full unit+golden+ml 4,850 passed / 14 failed vs the matched baseline at 028e508 (4,755 / 14) — failure sets identical, comm empty both ways. 39 sabotages, 39 behaved as declared. Armed ml tier 8/8 on the real checkpoint. ruff + black clean. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
Ten CodeRabbit-CLI rounds, 27 findings, all 27 real (6 major), closing with "No new findings". The sequence 9, 4, 3, 1, 2, 2, 1, 2, 5, 0 did not decay monotonically — r9 carried two majors after four quiet rounds — so a small round was never evidence of convergence, and that is the part worth carrying forward. Records the recurring shape (a malformed field escaping as a traceback instead of the named refusal + exit 3, in four places), the two emptiness defects, r9's two majors, and the three rate-limit blocks totalling ~111 minutes of waiting. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
This was referenced Aug 7, 2026
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.
Builds the backend for the mining spare rule's fourth disjunct (ADR-0005 D14). Its predicate has been pinned since P2-07 with nothing to feed it: the round-0 FP manifest carries coordinates, and
remine.load_stage2_posteriorswantscandidate_id → posterior.What it does
Composes shipped primitives and re-derives none of them:
payload[payload["gated_posterior_key"]], never a literal, so the indirection has one implementation.gate.calibration.temperatureout ofreports/gate2_p3_ece.json. A test asserts no module insrc/hardcodes it.stage2/dataset.pyraises on a non-zero flank, so the head was trained and its temperature fitted on bare loci; a flag here would let a run decalibrate every value it publishes while every gate stayed green.0.0. Absent resolves tounavailable⇒ spared; a literal zero resolves tofailed⇒ mined.The strand decision (§7, resolved on measurement)
The manifest carries no strand and Stage-1 is Caduceus-PS (RC-equivariant), so a Stage-1 window asserts no orientation. Both strands of all 941 were scored:
+The minus strand is higher on 458 / 941, and the 119 are not marginal (
+ = 7e-6vs− = 0.9997). A plus-only reading returnsfailedon them — a satisfied mining conjunct on loci Stage-2 calls near-certain T-boxes, which is what D14 exists to prevent. Signedmax_over_strands(bioedca, 2026-08-05).--strand-policyshipsrequired=Truewith no default; both per-strand posteriors ride in the report;posterior_kindstates in the artifact that a max of two draws is a selection on the object GATE-2 graded.The denominator travels with the numerator
load_stage2_posteriorsaccepts{}without complaint and[ -s ]passes on a two-byte{}, so a truncated table would certify as a clean round of nothing.mergerefuses below--min-coverage(default 1.0) and refuses a candidate claimed by two shards.STAGE2_SUPPLY_AVAILABLE = True, and it proves itselfderive_stage2_supply_available()re-derives it from six named, independently breakable, fail-closed clauses over git-tracked, non-DVC evidence only — so CI, laptop and cluster answer identically. That deliberately rules out a "checkpoint on disk" clause (DVC ⇒ False in CI) and replaces it with a control tied to the arm's own committed run report. The pin asserts agreement, not a literalTrue, so the constant cannot drift in either direction.remine's CLI gets the two-way flag pair + an exit-4 preflight: a barestore_truecould not express aTruedefault at all, which would have made both this flip and P3-15′-a constants nothing reads.Measured (laptop RTX 4060, all 941)
941/941 resolved, coverage 1.0000. 2.0 s resolve + 18.9 s load + 38.7 s scoring (1,882 rows, 0.0206 s/row), 1.33 GiB peak VRAM. Designed control: positive 0.999420, dinucleotide shuffle 0.000036, margin 0.999384 — against floors of 0.90 / 0.10 / 0.50 fixed before the measurement.
⚠ This does not move the yield
Mining is a conjunction.
relaxed_architectureanddownstream_aaRS_syntenystill have no backend, soplan_remine_roundreportsready=True,may_run=False,blocking = [relaxed_architecture, downstream_aaRS_synteny],max_mined = 0. A change that makes those two gates agree is a regression, and the test says so. P3-15′-c and P3-15′-d are the blockers.Validation
028e508(4,755 / 14) — failure sets identical,commempty both ways.run_controlfabricating its matchedness flags) targets the torch-only tier and was verified by execution against the real checkpoint, restore sha256-verified.sbatch --test-onlywas green while the job was not runnable, because the cluster env has no editable install and the sbatch lackedPYTHONPATH=src. Caught by executing the CLI ontwo, not by the preflight.The SLURM RUN is not submitted — it is at the §9.3 one-shot ack.
https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
Summary by CodeRabbit
New Features
Bug Fixes
Tests