Round two of external review: ten more, several in code I already called done - #12
Round two of external review: ten more, several in code I already called done#12aurascoper wants to merge 66 commits into
Conversation
Codex reviewed PRs #7-#10 after they were merged. I had not gone back for those comments, and every one of these was live on master. THE TWO THAT MATTER MOST, both in refusals I described as working: `scan()` ran the transport calculation and THEN validated the result. I wrote that it "refuses up front"; it did not — `sp = runner(build(cfg))` came before `per_source_from_statepoint`, so a misconfigured ladder burned the full sweep before saying no. Validation now precedes the runner. `authorization_criteria(baseline)` never received `sources`, so criterion 8 ("approval provenance recorded and retrievable") could not be met by any input. Demonstrated: a FULLY VALID approval left 1 of 9 unmet. The milestone was unreachable, which reads identically to correctly withheld — the failure mode is invisible precisely because the expected state is "not authorized". Sources are threaded through `readiness()` and `main()`; the same registry now returns 0 unmet. THE ONE THAT CHANGES A PUBLISHED NUMBER: 3.2 um does not tile the spheres' 24 um axis (7.5 voxels) and `_grid_shape` rounded it to 8, so that rung of the ladder rasterised a 25.6 um box while the closed-form truth kept dividing by the declared 24. A 6.7% denominator shift was being reported as rasterisation error, in the study whose entire purpose is to hold the object fixed and move only the sampling. Non-tiling pitches now raise `NonTilingPitchError` and the ladder names the pitch it skipped rather than dropping the row. Component size moves 3.2 -> 1.6 um; the interface-area finding is unchanged, having been measured where it tiles. The rest: - `approved_protocol_version` is a scope column, so blank hashed as blank and the digest still matched — an approval could bind to no protocol. Now in `_MUST_NAME_SOMETHING`. - `--publish` published results from a run that stopped early. - `_t_critical()` extrapolated below df 1; returns None there and a conservative tabulated value otherwise. - subvoxel refinement accepted ratios that do not divide. - `observer.plot_layer` rendered categorical fields as volumes. Ledger row LADDER-04 records the withdrawn rung. Suites: coupling 214 passed / 8 skipped, calibration 298, contract 7, Julia 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2913d26ee5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
All four verified before fixing; each fix carries a negative control that fails without it. THE TWO P1s, both in code I wrote this session: `plot_layer` referenced CATEGORICAL and BOOLEAN, which live in viewer.py and were never imported. Every call on a machine with a renderer raised NameError on the first line of the branch. Nothing caught it -- pyvista is installed neither in CI nor here, so all eight render tests SKIPPED and the suite reported green over a dead function. "8 skipped" was never a neutral line in that report; it was the uncovered surface. The import is one line; the test that resolves every global the render path uses is the point, and it needs no renderer. `authorization_criteria` mapped gate refusals to the nine criteria by substring and treated an unmatched refusal as MET. So the approved_protocol_version check I added last commit produced a refusal the milestone could not see: the gate said no while 0 of 9 criteria were unmet. An institutional biosafety milestone read AUTHORIZED over a live refusal -- the exact inverse of the bug it was meant to fix, and worse, because a false positive here points the wrong way. Unmapped refusals now fail criterion 6 and are named. A parametrized control breaks each approval field in turn and requires every refusal to land somewhere. AND MY FIX FOR THE LADDER WAS ITSELF WRONG. Removing 3.2 from the default pitches is not the same as refusing it: 3.2 tiles the SLAB, so that axis-aligned control row vanished, and run_ladder only emits its `skipped` row for a pitch it is handed -- so the promise that the ladder names what it skipped became unreachable in the very commit that made it. A pitch absent from the list is indistinguishable from a pitch nobody tried. 3.2 is back; the slab evaluates it, the spheres refuse it by name. The component_size 3.2 -> 1.6 um correction is unaffected. Fourth: a blanket threshold at 0.5 deleted every zero-valued categorical cell. cell_id 0 is empty space, but the generation layer's own note reads "generation 0 is a founder, not a missing value" -- the producer wrote the warning and the renderer ignored it. Layer.background is now declared by the code that built the field; None means no cell may be hidden. THE GATE. Codex has raised a P1 on four consecutive PRs. The reviews were never the missing piece -- nothing stopped a merge while they sat unread, which is how ten issues reached master across #7-#10. CI cannot close this: it answers whether tests pass, not whether anyone read the review. scripts/preflight_merge.sh refuses while any thread is unresolved, and refuses a review whose commit is no longer the head. Run against #12 as it stood, it exits 1 naming all four findings. AGENTS.md and CLAUDE.md did not exist at repo root. One shared body of standards -- CLAUDE.md imports AGENTS.md -- so two reviewers are held to one bar. Six rules, each written because it was violated here and shipped. Ledger rows OBS-01, REFD-05, LADDER-05, VIEW-03. Suites: coupling 215 passed / 8 skipped, calibration 312, contract 7, Julia 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review All four findings addressed in be75b67. Each fix carries a negative control that fails without it — I checked that by reverting each one and confirming the test goes red:
Also added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be75b67a2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…the bug it exists to catch Two more from the re-review, both on code from the previous commit. THE OVERSHOT FIX. `generation` is 0 for a founder AND for empty lattice sites -- `export_checkpoint.jl` zero-fills the array and skips unoccupied voxels -- so NO background value can separate them. Thresholding at 0.5 deleted the founding cohort; declaring `background=None` to fix that drew the void as generation-0 biomass. Both shipped, in that order, the second as the correction for the first. A value that cannot disambiguate itself needs a different field to do it. `Layer.occupancy_from` names the layer that says where biomass is, and only the producer knows which one that is. `generation` and `lineage_id` name `cell_id`; `cell_id` keeps `background=0` because it is self-describing. The mask uses the PRE-EXISTING `observer.occupied_mask`, which tests `> 0` rather than `!= 0` -- the schema puts -1 outside the biological domain, so a truthiness test would have drawn the wall as biomass. That helper was already in the file, documenting exactly this trap, and my first pass wrote `!= 0` next to it. THE GATE HAD THE BUG IT EXISTS TO CATCH. `preflight_merge.sh` listed `P1|P2|UNRANKED` as its failing cases, so a pull request whose only open thread was P3 printed "Clear to merge" while the script's own header said it refuses while any thread is unresolved. A check that could not fail, inside the check written to stop checks that cannot fail. Severity ranks what to fix first; it does not rank what may be ignored. Every severity now blocks. The gate takes a PREFLIGHT_FIXTURE so its decision logic is testable without GitHub, and ships eight controls that run in CI as `merge-gate`: six inputs it must refuse -- P1, P2, P3, unranked, a review naming a commit that is no longer the head, and no review at all -- and two it must accept, so a gate that refused everything could not pass its own suite. Reverting the severity list reproduces "Clear to merge" over an open P3. Codex cited AGENTS.md line ranges in both findings, which is the shared standards file doing the work it was added for. Ledger rows VIEW-04, GATE-01. Suites: coupling 216 passed / 8 skipped, calibration 312, contract 7, Julia 9, merge gate 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Both round-three findings fixed in 4355cda. You were right on both, and the first one was a fix of mine that overshot. Occupancy mask — verified your claim against One thing your comment led me to that you didn't ask for: the mask now uses the pre-existing P3 in the gate — correct, and it's the worst version of it: a check that could not fail, inside the check written to stop checks that cannot fail. Every severity blocks now. Severity ranks what to fix first, not what may be ignored. You asked for a negative control with a synthetic unresolved P3, so the gate now takes Also: you cited |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4355cdaba1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ate itself All three verified before fixing; each carries a control that goes red when the fix is reverted. A DANGLING OCCUPANCY REFERENCE FAILED OPEN. Nothing validated `occupancy_from`, though the `source_grid_id` check three lines above has existed all along. A misspelt name does not merely mislabel a layer -- it drops the renderer straight back into the founder-versus-void ambiguity the field was added to resolve, silently. Refused at write time now, for three cases: a name that is not a layer, a layer on a different grid (a mask must be cell-for-cell with what it masks), and a layer declaring both a background value and an occupancy layer, which is two answers to one question. THE GATE FAILED OPEN PAST A HUNDRED THREADS. `reviewThreads(first:100)` with no pagination: unseen threads are indistinguishable from resolved ones, so a long-running pull request would have cleared while carrying unread findings. A gate that silently truncates its input is the same defect it exists to catch, and this repository's own rule is that a bounded scan must say what it dropped. Now walks every page. Verified against PR #12 itself -- 9 open threads seen, exit 1. THE PUBLISH GUARD HAD NEVER RUN. `--publish` overwrites the canonical evidence tables in data/calibration/, and the only thing stopping a timed-out run from doing that was two lines inside `_report` -- which imports openmc on its first statement and therefore cannot execute anywhere in this suite. The protection existed and had never once been exercised. Extracted to `refuse_partial_publish` and tested over all four publish/stopped_early combinations, not just the refusing one: a guard that raised unconditionally would also stop the bad case, and would make the pilot unpublishable. The two P1s still open on the PR are the ORIGINAL 23:33 review carried forward and re-anchored, not re-raised -- checked by comment timestamp before assuming either way. Resolving them next, which is the record that they were read. Ledger rows VIEW-05, GATE-02, PILOT-07. Suites: coupling 223 passed / 8 skipped, calibration 312, contract 7, Julia 9, merge gate 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Round four fixed in eac4e6c. All three were guards that could not fail, and one of them was the gate itself. Dangling Pagination — this was the worst of the three, because it was in the gate. Publish guard — worth stating plainly: it had never executed. It sat inside On the two P1s still showing open — I checked comment timestamps before assuming either way. Both are from the original 23:33:36Z review, carried forward and re-anchored as the files changed (observer.py 259 → 265 → 273), not re-raised. I've now verified all six earlier findings against HEAD and resolved those threads:
One note on that last check: my first grep for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eac4e6c427
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ame check could catch exactly one bug
THE PALETTE BROKE THE THING IT EXISTS FOR. `species_legend` correctly
returns only the species present, each with its fixed colour -- but
handing those straight to the renderer with clim spanning the id range
DISTRIBUTES them across it. For ids {1, 2, 7} three colours spread over
six units, so species 2 drew in whatever fell at that fraction while the
legend labelled it with the second palette entry. The picture disagreed
with its own key, which is the exact failure the seven fixed colours were
introduced to prevent.
`species_palette` now covers the contiguous id range with half-unit
padding, so each id sits in the middle of its own band. Extracted from the
render branch so the bare tier can reach it, and tested as the invariant
rather than as an output: the same organism must land in the same slot
under {sid}, under {1,2,7}, and under {3,4,5}.
MY NAME CHECK COULD CATCH EXACTLY ONE BUG -- the one in front of it. It
filtered `co_names`, which mixes global lookups with attribute names, by
requiring a name be UPPERCASE and an actual export of viewer.py. So
`CATGORICAL` (not a viewer export) and `occupied_maks` (not uppercase)
both sail through while `plot_layer` raises NameError on any host with a
renderer -- the precise skipped surface the test claims to cover. A check
narrowed until it matches one known defect has stopped being a check.
The bytecode already draws the distinction, so ask it: LOAD_GLOBAL
instructions, over every function defined in the module rather than a
hand-listed six, plus a known-bad function proving both misspelling shapes
are caught.
THE FIXTURE HOOK HID THE PAGINATION FIX. Every gate control sets
PREFLIGHT_FIXTURE, which bypasses the `gh api` branch entirely, so
removing --paginate, $endCursor, pageInfo or the jq fold left all eight
green -- the mechanism that made the gate testable concealed the newest
thing added to it. A fake `gh` on PATH now emits two pages with the only
unresolved thread on page two. Removing --paginate reproduces "Clear to
merge" over an open P1.
That is the second finding in a row against the merge gate itself, which
is the correct amount of scepticism to point at a thing that decides what
may ship.
Six earlier findings verified against HEAD and their threads resolved;
timestamps confirmed the two P1s were the original review re-anchored, not
re-raised.
Ledger rows VIEW-06, TEST-01, GATE-03. Suites: coupling 225 passed / 8
skipped, calibration 312, contract 7, Julia 9, merge gate 9.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Round five fixed in c48e3bd. All three correct; two were in code whose whole purpose was to prevent the failure it exhibited. Species palette — this is the sharpest one. The name check — you're right, and worse than the example shows. It filtered Now reads Pagination control — correct, and the irony is precise: That's two consecutive findings against the merge gate itself, which is about the right amount of scepticism to aim at the thing deciding what may ship. Round-four threads verified at HEAD and resolved. Suites: coupling 225 / 8 skipped, calibration 312, contract 7, Julia 9, merge gate 9. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c48e3bde65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…missed being deleted Codex is now applying this repository's own first rule to the fixes I made three rounds ago, which is the right thing to point at them. Three guards I added in the round-two commit had no negative control, and two were unreachable by any suite that runs here. `scan()` REFUSES BEFORE THE RUNNER -- but the test called `_biofilm_scan_mass` directly, so it passed whether the preflight loop existed or not. Delete the loop, `scan()` reverts to paying for the transport and refusing afterwards, and nothing turns red. Now driven through `scan()` with a spy runner that raises if reached. `_t_critical` and the ratio-divisibility check were both nested below `import openmc` -- inside `_report` and inside `main()` respectively -- so neither had ever executed in this repository. Both extracted to module level and tested. WORTH RECORDING BECAUSE IT ALMOST PRODUCED A FALSE CONCLUSION: my first negative control for the low-df refusal PASSED against a broken function, and I nearly wrote it up as covered. Two reverts cancelled: disabling the `df < 1` early return leaves the table fallback returning None anyway, so the observable behaviour was unchanged. The branch only matters against the regression it was written for -- substituting the normal critical value 3.09 -- and that is what the control now targets. A control that cannot distinguish the fix from the bug is not a control, which is the same rule one level up. The ratio guard is tested in both directions, since a guard refusing everything would also stop `1,2,4`, the ladder the study actually runs. Fourth, on the palette fix from last round: a species layer of pure background yields an empty legend -- which the existing out-of-range test already permits -- and `min()` over nothing raised ValueError before the layer could draw. A viewer that crashes on an empty snapshot cannot show that the snapshot is empty. Ledger rows DRV-02, PILOT-08, REFINE-02, VIEW-07. Suites: coupling 238 passed / 8 skipped, calibration 312, contract 7, Julia 9, merge gate 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Round six fixed in 4c34fe7. All four correct — and you're now applying this repo's first rule to fixes I made three rounds ago, which is exactly where it should be pointed.
One thing worth recording, because it nearly became a false conclusion. My first negative control for the low-df refusal passed against a broken function and I almost reported it as covered. Two reverts cancelled out: disabling the A control that can't distinguish the fix from the bug isn't a control. Same rule, one level up. Empty species layer — confirmed, Round-five threads verified at HEAD and resolved. Suites: coupling 238 / 8 skipped, calibration 312, contract 7, Julia 9, merge gate 9. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c34fe72d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
One correctness bug among the round-seven findings, and it is a
misdirection rather than a silence.
`approval.problems` emits "<field> is unset" for the three date fields AND
for `approval_source_id` and `approval_scope_hash`. Criterion 7 matched the
bare phrase, so an approval with no registered document reported criterion
7 -- ABOUT DATES -- as its blocker, while criterion 8, about provenance,
stayed green. Whoever read that milestone would have gone to fix a date
that was never the problem.
Criterion 7 now names the three date fields explicitly; `approval_source_id
is unset` maps to criterion 8. Verified per field: source id -> 8, scope
hash -> 2, each date -> 7, and nothing falls through to the unmapped catch.
THE FIRST VERSION OF THE CONTROL DID NOT CATCH IT. Asserting the right
criterion is PRESENT cannot detect an EXTRA one, so with the bare pattern
restored the test passed while criterion 7 was still being wrongly
reported. It now also asserts the criteria that are not implicated stay
met, and reverting reproduces:
AssertionError: criterion 7 is reported unmet, but
{'approval_source_id': ''} has nothing to do with it
That is the second time this session a negative control passed against the
bug it was written for. Both times the cause was the same: the control
tested that the fix works, not that its absence shows.
Ledger row REFD-06. Suites: coupling 238 passed / 8 skipped, calibration
317, contract 7, Julia 9, merge gate 9.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The empty-palette fix only moved the crash one line down. Thresholding an all-background layer leaves a dataset with zero cells and `add_mesh` rejects it, so `ValueError` from `min()` became a PyVista error instead. `plot_layer` now skips the mesh call on an empty dataset and says "no occupied cells" on the canvas, because "nothing is here" is a result a reader needs to see. WHAT MATTERS MORE IS WHY I DID NOT CATCH IT. The bare-tier control exercised `species_palette([])`, verifying the fix one level below where it failed -- and it could not do better, because pyvista was installed nowhere that runs this suite. Eight silent skips hid three real defects in a single session: a NameError on the first line of the render branch, a species palette that mis-slotted every id under an uneven subset, and this crash. All three were found by external review. None was found here. So pyvista joins the dev extra and CI runs the tier under xvfb, with -rs so the skip list stays visible in the log. Locally: 241 passed, 6 skipped, and every remaining skip is openmc, not the renderer. The render-path work from the last five commits has now actually executed for the first time -- the import fix, the occupancy mask, the palette slots and the empty mesh all pass against a real renderer rather than against my reasoning about one. "N skipped" is not a neutral line in a test report. AGENTS.md said so three commits before this one, and the suite went on not running. Ledger rows VIEW-08, TEST-02. Suites: coupling 241 passed / 6 skipped, calibration 317, contract 7, Julia 9, merge gate 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Round seven in a5ff1c5 and 7519292. The criterion mis-attribution was the real bug here and you're right that it's worse than a silence. An unset Worth recording: my first control for it passed against the restored bug. Asserting the right criterion is present cannot detect an extra one. It now also asserts the criteria that aren't implicated stay met, and reverting reproduces Empty species mesh — correct, I only moved the crash from And the reason I kept verifying these one level below where they failed: pyvista was installed nowhere that runs this suite. Eight silent skips hid three real defects in one session — the NameError, the mis-slotted palette, and this crash. All three found by review, none by the suite. So pyvista is now in the dev extra and CI runs the tier under Every remaining skip is openmc, not the renderer. The render-path work from the last five commits has now actually executed for the first time. Your four remaining P2s are one class — exercise the guard through |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75192927bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Five findings, one class: I had tested the extracted helper and called the guard covered. Deleting the CALL leaves the helper test green in every one of these. `refuse_partial_publish` -> the canonical directory and the refusal now live in one function, so `data/calibration/` cannot be obtained without passing the guard. The refusal is the door, not a sign beside it. `t_critical_999(0) is None` proves the table, not that the None reaches the verdict. `distinguishable_from_zero(e2, se, m)` is now the tested unit; the case that matters is m=2, se=1.0, which must FAIL because at one degree of freedom the critical value is 318.3, not 3.09. THE RATIO GUARD WAS NOT ACTUALLY EARLY. It sat below `import openmc` and below `load_snapshot`, so a bad ratio surfaced only once the heavy stack was up -- and never in this tier, since the import fails first. Parsing ratios needs nothing but the argument string, so it moved above the import. `main()` is now driven with `1,3,4` directly, and the control fails both when the call is deleted AND when it is moved back below the import. The gate's pagination fake emitted page two on seeing `--paginate` alone, so deleting `$endCursor` or the `pageInfo` selection left it green while real pagination stopped -- the same fail-open shape the pagination fix existed to close, reproduced inside its own test. It now rejects a query missing either element. Third finding against the merge gate; it has earned the scrutiny. AND AN IDENTIFIER COULD MANUFACTURE A DATE BLOCKER. Matching against `" ".join(problems)` meant any echoed field value could trip an unrelated criterion: `approval_source_id = "expired"` gave a provenance-only refusal whose text contained the word, and criterion 7 fired on it. Each message is now classified by the field named at its head, cross-field sentences by phrase, and `approval_scope_hash` splits by message -- unset means nothing binds the row (2), a mismatch means the approval no longer covers it (6). Verified across seventeen single-field breakages: each yields exactly one correct criterion. Ledger rows REFD-07, PILOT-09, PILOT-10, REFINE-03, GATE-04. Suites: coupling 252 passed / 6 skipped, calibration 323, contract 7, Julia 9, merge gate 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Round eight in c3575b1. All five correct, and they were one class: I'd tested the extracted helper and called the guard covered. Deleting the call leaves the helper test green in every one of them.
All threads resolved. CI green including the render tier at 252 passed / 6 skipped, all six openmc. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3575b1277
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two findings, both refinements of yesterday's fixes, both correct. THE ECHOED-VALUE HOLE WAS NOT CLOSED, ONLY NARROWED. I checked the scope-mismatch phrase BEFORE reading the message head, so `approval_source_id = "does not match the conditions"` produced a provenance-only refusal that the checklist reported as criterion 6 -- leaving criterion 8, the actual defect, marked met. An identifier could still redirect the institutional checklist; it just needed a longer string. A phrase may narrow a message already known to be about a field. It may never decide which field a message is about. The field is read first now, and the scope phrase only splits `approval_scope_hash` between "nothing binds this row" (2) and "the approval no longer covers it" (6). THE CURSOR CHECK PASSED ON THE DECLARATION ALONE. Requiring `$endCursor` to appear ANYWHERE is satisfied by the variable declaration, so deleting `after:$endCursor` from the connection left all nine controls green while cursor updates could no longer advance `reviewThreads` -- the real gate would stick on page one. Codex verified this by removing only the binding and watching the suite pass. The fake now requires the binding itself, and removing it fails the control. That is four findings against the merge gate. Every one has been a fail-open in the thing whose entire job is to refuse. Ledger rows REFD-08, GATE-05. Suites: coupling 252 passed / 6 skipped, calibration 325, contract 7, Julia 9, merge gate 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Round nine in bb542a3. Both correct, and both are the same hole one size smaller. The phrase override ran before the head read. So The rule now is explicit: a phrase may narrow a message already known to be about a field; it may never decide which field a message is about. Field first, then the scope phrase splits The cursor check passed on the declaration alone — thank you for actually running that. Requiring That's four findings against the merge gate, and every one has been a fail-open in the thing whose entire job is to refuse. Worth stating plainly rather than filing away. All threads resolved. Suites: coupling 252 / 6 skipped (all openmc), calibration 325, contract 7, Julia 9, merge gate 9. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb542a3574
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dict `approval.problems` formats the growth condition id with `!r`, and Python switches to double quotes when the value contains an apostrophe. So an ordinary id like `O'Brien-1` produced a prefix the classifier's regex could not strip: the head became the word "condition", nothing classified, and an unset source id was reported as a scope failure plus an UNMAPPED refusal. Whether a growth condition happens to be named after someone Irish is not a fact about its biosafety approval. This is the third successive narrowing of one defect -- data reaching a decision it should have no part in. First the concatenated text let any echoed value trip an unrelated criterion; then the phrase override let a longer echoed value do the same; now the id itself, without any adversarial content at all, just an apostrophe. Each fix was correct and each left the shape intact one size smaller. Both repr quote styles are stripped now, and the regression test runs five ids including an apostrophe, a double quote, a space and a slash, asserting the same four verdicts for each. Ledger row REFD-09. Suites: coupling 252 passed / 6 skipped, calibration 330, contract 7, Julia 9, merge gate 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Round ten in 65a1d97. Confirmed and fixed.
This is the third successive narrowing of one defect — data reaching a decision it has no part in. First the concatenated text let any echoed value trip an unrelated criterion. Then the phrase override let a longer echoed value do the same. Now the id itself, with no adversarial content at all, just an apostrophe. Each fix was correct; each left the shape intact one size smaller. Worth naming as a class rather than patching a fourth time. Both repr quote styles are stripped now, and the regression runs five ids — apostrophe, double quote, space, slash, plain — asserting the same four verdicts for each. Reverting to the single-quote regex fails on Whether a growth condition is named after someone Irish is not a fact about its biosafety approval. All threads resolved, CI green. Suites: coupling 252 / 6 skipped (all openmc), calibration 330, contract 7, Julia 9, merge gate 9. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65a1d97fe2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Said it was deliberately not committed in 65ce76c, then `git add -A` in the next commit swept it in anyway. The .tex is the source; add a gitignore rule so the next `-A` cannot repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0af28558fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`push` here is branch-filtered to master/feat/ci/research and there was no `pull_request` trigger, so a PR from `fix/**` -- or from any fork, which matches no branch filter -- never fired this job, while coupling-tests.yml went green replaying the committed tally. A change to a fixture-producing module could merge with the pin it invalidated never revalidated: green against something nothing rechecked. model-contracts.yml already mirrors its push paths into a filtered pull_request; this file should have copied that and did not. I noted the branch-filter gap out loud when I pushed the workflow and then did not close it. The two path lists are one list via a YAML anchor, not two copies. Copies drift, and the one that drifts is the pre-merge trigger nobody watches. AND THE OLD TEST WAS WEAKER THAN IT CLAIMED. It searched the file TEXT for each producer path, so a path listed under `push:` alone satisfied it -- it would have passed against exactly the configuration that shipped this bug. It now parses the `on:` block and requires every producer in BOTH filters. Asserting the paths while never asserting the EVENT is the hole that let this ship, so there is a second test for the event itself, and it checks the two lists are the same object rather than equal today. pyyaml moves into [dev]. It resolved only transitively, which works until whatever pulls it stops -- the same argument the comment above it already makes for pyvista. Controls: deleting the pull_request trigger fails both tests; de-anchoring into two equal-but-separate lists fails the identity assertion alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f5b79db5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
THE CI PATH. golden-tally-verification's first real run failed after 12m: the download step writes to $HOME/openmc-data (/home/runner) while both consumers hardcoded /root/openmc-data. /root is mode 700, so openmc's import died on EACCES -- 6 collection errors. Two hand-written copies of one path, in the same file that had just grown a YAML anchor to prevent exactly that, in the same commit. The download step now exports the path via $GITHUB_ENV after checking it is readable, and both env: blocks are gone. What DID hold: the job failed loudly instead of reporting green, and the assert-tests-actually-ran step was skipped rather than passing on an empty run. THE FIXTURE ASSERTED PRECISION IT DOES NOT HAVE. It stored 16 significant digits per cell. Measured on the fixture itself, the tally's Monte Carlo relative sd has a median of 27% per cell (min 3.8%, max 100%). Digits past the sixth are not measurement -- and `git diff --exit-code` required every one of them to reproduce on another machine. The precedent this fixture cites, tests/fixtures/serial_seed42.csv, pins 5 decimals with a header saying a different Julia minor version may legitimately change the bytes: ROUNDING IS THE TOLERANCE, and I claimed to follow that pattern without following it. Now 6 significant figures, ~5 orders below the noise, with the fixture header naming what legitimately changes the numbers. Regenerated for real -- 12 OpenMC runs -- then checked all 13,312 values against the OLD file rounded the same way: identical. That proves the rounding is presentation-only AND that the runs reproduce bit-for-bit on this machine, which is the precondition for the cross-machine claim the diff step makes and which nothing had ever checked. 361KB -> 239KB. THE ABSTRACT was 8 sentences with the first caveat in sentence 2 and no clean positive sentence anywhere; every accomplishment was yoked to a limiting clause in the same sentence. Resequenced into four paragraphs -- what was built, what was measured, what the boundaries are, what it enables -- and surfaced the novelty the paper already establishes at 2.4 and never put in the abstract: no prior coupling of OpenMC to a CPM or any cell-scale biological model exists. It travels with its own qualifier, as 2.4 carries it, because the fact without "extends an established paradigm rather than introducing it" would be a larger claim than the paper makes. NOTHING is deleted, softened, or hedged less; every caveat survives, and each abstract claim was traced to the passage that already establishes it. THE INTRODUCTION oversold where the rest of the paper does not. "comprehensive mathematical framework" is contradicted by 3.2 in the same document -- "four of the nine terms are unrepresentable rather than merely unimplemented" -- and "we simulate and explore the spatiotemporal evolution of these communities" is broader than what runs. Both scoped, with a forward reference to where the gaps are marked. Softening the abstract while leaving these would have made the two ends of the paper disagree about the same work. Bibliography count still exactly 15 unused, the \cite count is unchanged, and no claim moved -- so no claims_ledger row. PDF rebuilt: 26 pages, 0 overfull hboxes, 0 undefined references. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
Manifest.toml is gitignored (.gitignore:377). It exists in my working tree at 76 KB and `git ls-files` does not list it, so "declared in Project.toml and resolved in Manifest.toml" is true of the tree it was written in and false of every clone: a fresh checkout has no resolved dependency graph, and Pkg.instantiate() resolves against whatever the registry offers that day. Claiming a lock implies a reproducibility this repo does not currently provide. Same error one screen up: the repo-tree listing showed "Project.toml, Manifest.toml, LocalPreferences.toml" as repository contents. Only the first is tracked. Checked all three rather than the one that was reported. This is the presence-claim mirror of an absence claim that names only the place someone looked -- the fact was verified against a working tree and stated about the repository. Codex also flagged regenerate_golden_tally.py's provenance for naming only OpenMC and nuclear data as legitimate fixture inputs; that was already fixed in cd017c4, which added what_legitimately_changes_these_numbers naming config, model, mesh, materials, dose and the script itself. Verified in the committed fixture before resolving rather than assuming the commit covered it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb908e511d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ence Codex P1, on a sentence I added two commits ago: "every parameter is a literature-anchored prior rather than a fit." That is false, and it contradicts this repository's own audit ledger. TWO ERRORS IN ONE SENTENCE. Section 2.6 says "Every parameter IN TABLE 2 is therefore a literature-anchored prior" -- I lifted it into the abstract and dropped the scope. Table 2 is not every parameter: the transport tier's provenance file records evaluated_nuclear_data, direct_measurement and manufacturer_datasheet rows, categories that are not literature priors and several of which are stronger than priors. And the scope I dropped would not have saved it, because the parameter that matters most is not in Table 2 at all. The melanin acceptance term hard-codes a coupling of 0.5 against the local melanin field (biofilms_potts.jl:568,571) instead of the declared per-species alpha_M, and that hard-coded value is the coefficient through which radiation actually reaches the CPM dynamics. No citation, no table row, no config file. data/claims_ledger.csv RM-G04-01 has said exactly this since it was written, and PP-T2-29 carries it as flagged_weak. My sentence asserted the opposite of a fact this repo had already audited and recorded. The abstract now states the real split -- most literature priors or derived from them, several flagged weak, and the dominant melanin coupling a declared modelling choice -- which is stronger than the blanket claim, not weaker. AND IT EXPOSED A GAP IN THE BODY. Section 4 carries four provenance declarations and none of them discloses the hard-coded coupling; an abstract must not carry a fact the body lacks. Added the declaration where the others live, and said why it matters: the 15.5% acceptance shift is this coefficient's effect, so the largest measured quantity in the manuscript rests on the least evidenced parameter in it, and a reader who assumed the table covered every parameter could not learn that from the table. The ledger did its job here; the manuscript had not caught up. No new ledger row: RM-G04-01 and PP-T2-29 already record the fact. What changed is that the manuscript now reflects them. PDF rebuilt, 27 pages, 0 overfull, 0 undefined refs. Bibliography still exactly 15 unused. Julia and calibration suites green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd27588422
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The phase-one biosorption isotherm is blocked twice over, for unrelated reasons, and both are recorded now rather than discovered once there is a number to defend. NO COUPON. D-RHOWET requires "paired wet mass and hydrated volume on the SAME coupon"; D-BLANK a matched blank coupon; D-DRY dry mass on the same coupons. bulk_measurements.csv is built around coupon_id, imaged_area_cm2, weighed_area_cm2, scaling_method. Suspended biomass has no coupon, so a suspended measurement has no expressible row there. The caveat is not that suspended and biofilm material differ -- it is that the number cannot enter the table at all. NO UNITS IN THE SOLVER. biofilms_radiodialysis.R:161 is `c_ext = 1.0 # normalised`. A q_max in mg/g needs rho_dry -- itself a prior until phase-two hydrated volume exists -- AND a reference concentration that exists nowhere here, before it can enter the transport equations. IT IS A SEPARATE FILE, NOT A REGISTER ROW, and that is forced: reference_d_requirements.csv is cross-checked against required_fields() in BOTH directions, so a row naming a field the emitter does not require is a failure. A suspended capacity has no config_key. The repo already has the right vehicle -- reference_d_condition_proposal.csv, PREPARATORY NOT AUTHORITATIVE, every row authoritative_for_campaign = false, isolation enforced by a test rather than asserted in a comment. Same shape here. Also recorded, because each would otherwise be relearned: k_ads is not derivable from b (b is the RATIO k_ads/k_des at equilibrium, and a ratio gives neither rate -- an isotherm alone yields q_max and b, a rate needs timed sampling); X_max = q_max * rho_dry multiplies a measured value by a prior and inherits the weaker class; imaging does not escape the protocol question, because D-RHOWET refuses cells_only by name and a z-stack segmented on a cell stain IS cells_only; and detectability-against-blank comes BEFORE the wet/dry CV, which comes before replicate sizing. AND THE REGISTER MISDOCUMENTED ITSELF. Its preamble named not_started, blocked_on_model and awaits_upstream; load_requirements() accepts none of them, so a row written by following the file's own instructions could not load. Invisible for exactly as long as nobody added a row. Code is the contract, so the prose was corrected to it -- naming the fork, because the two repairs look identical and mean opposite things: had those been an intended design, deleting them would have discarded it. unsupported_by_current_model already covers blocked_on_model, so at most awaits_upstream was lost. Both new tests verified against known-bad input: restoring the stale vocabulary fails the drift check, flipping a proposal row to authoritative_for_campaign = true fails the isolation check. Calibration suite 347 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
The package list lived in docs/openmc_stack.md and inline in the create-args of both workflows. Two bugs came straight out of that arrangement: a cross-sections path written twice (/root vs $HOME, which failed the job's first real run) and a paths: filter covering less than its own comment claimed. Three hand-maintained copies of one fact drift, and the copy that drifts is the one nobody is looking at. Both workflows now read environment.yml. The doc NAMES the file instead of restating the list, because a quotation is a fourth copy -- and the test asserts exactly that, refusing a doc that mentions openmc=0.15.3 at all. This is a correctness fix rather than tidying: CI regenerates the golden tally and diffs it, so a spec that differs between local and CI surfaces as fixture drift with no obvious cause. vtk joins the spec. openmc's own mesh.write_data_to_vtk imports it function-locally (openmc/mesh.py:618) and raises ModuleNotFoundError without it, so the export half of an export/render split cannot run in the pinned env as it stood. I HAD CLAIMED THE GOLDEN TALLY WOULD DETECT ANY NUMERICS CHANGE FROM THE moab REBUILD. That was an assumption about a test's coverage, and it was wrong: grep for dagmc/DAGUniverse/.h5m across coupling/ returns nothing. The geometry is pure CSG, so moab is linked into openmc's build but is not in the transport path -- making the rebuild simultaneously undetectable by that test and harmless, both from the same fact. Verified rather than argued: installed vtk, re-ran all 12 real OpenMC transport runs, and the fixture came back byte-identical. libnetcdf 4.10.0 -> 4.10.1 is the only real version change in the solve and is named in environment.yml as the one to watch. The single-spec test parses the workflow YAML rather than grepping it. A substring scan for "create-args" fired on the comment explaining that create-args had been removed -- a check failing for the opposite of its reason, and the same lesson the paths-filter check already learned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
synthetic_e2e.py -- the coupling this repository actually reports -- called write_bundle nowhere, so its dose and labels got none of the coordinate checking bundle_problems applies: grids in one bundle must describe the same physical volume and share one origin. They were registered only by index arithmetic. It now writes a two-grid bundle from what the run has already computed, costing no transport, pure numpy+h5py so the pinned env is untouched. The lattice-resolution dose field is written authoritative_for_quantitation=False with derivation=upsampled_coarse_dose, because that is what it is: one coarse value repeated across each bin. WHY WORLD COORDINATES AND NOT INDICES. Axis permutation is already well covered -- four asymmetric lattice probes, an adversarial transposed-axis refusal, and a Julia round-trip encoding x+10y+100z. What is uncovered is that there are TWO transforms wearing one name: RectLattice.universes transpose(2,1,0)[:, ::-1, :] HAS a y-flip RegularMesh tally reshape(dim[::-1]).transpose(2,1,0) NO y-flip The first lives in snapshot.to_openmc_lattice_order and is probe-verified. The second is reimplemented independently in dose.extract_heating and again in materials.mesh_material_masses_kg, neither probed, both commented "the same convention" -- which is related, not same. Nothing asserted that the voxel hot in the tally is the voxel holding biomass in the lattice. docs/exchange_schema.md claimed the mapping "lives in exactly one Python function"; true of the lattice direction, and it read as covering the whole coordinate story. Corrected to say which one, and to name the two that are not it. NOT A CENTROID, and this is the part I had wrong. A dose-weighted world centroid against an occupied-label centroid cannot detect a y-flip when the field is symmetric about the flip axis -- a flip preserves the first moment. That is invariant 7's weakness restated one level up: the exact failure the check exists to close. So the test compares located values at asymmetric probes, and asserts its own fixture's asymmetry as an explicit precondition, because otherwise the fixture becomes the load-bearing part and a later edit disarms the check with nothing failing. Controls: flipping the dose field on each of the three axes is caught independently; a shifted grid origin -- every index still resolving, every extent still matching, only the physical placement wrong -- is refused outright; and making the fixture symmetric or giving SHAPE a repeated extent fails the precondition AND all three flip controls, so a disarmed fixture cannot pass quietly. coupling 307 passed, Julia green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
subvoxel_refinement.py:362 says why its two resampled layers exist: "BOTH SIDES OF THE RULE, on real data, because a viewer overlaying dose on labels needs exactly these two." Nothing ever drew that overlay. plot_layer renders one layer per call and nothing composited; no code path drew any surface geometry at all. plot_overlay composes rather than reimplements -- plot_layer already accepts a plotter, and both layers are positioned by to_image_data from their own grid's declared origin_cm/spacing_cm, so the two grids share a world frame BECAUSE the bundle says where they are. If one is offset the picture shows it offset, which is the point. Nothing here recomputes a coordinate, and the module still computes no metric. THE PICTURE IS NOT THE CHECK. test_grid_coregistration.py is what fails when the grids disagree; this is what a reader opens afterwards to see how. A picture nobody is obliged to look at cannot be a gate, and pretending otherwise would be the check-that-cannot-fail this session keeps finding. The CSG cylinder is drawn wireframe, unlit, labelled "not data" on the canvas: the rectilinear lattice sits inside a curved boundary, so voxels at the wall are partially inside -- which is why mesh_material_volumes raytraces them rather than counting bins. Seeing which voxels those are is the boundary's whole diagnostic value, and it must not read as a field. Two controls. The style assertion requires BOTH Wireframe and Surface present, so it cannot pass on a scene that drew only the boundary. And the overlay must not be able to suppress a non-quotable layer's banner: it composes plot_layer, so it inherits that refusal, and the test asserts the inheritance because a composite quietly passing show_banner=False would undo the one rule the display layer has. Two things I got wrong and fixed rather than worked around: I wrote an `assert any(... or True ...)` line, which is literally a check that cannot fail, and deleted it. And the style check first used `prop.representation`, which is raw VTK -- pyvista raises PyVistaAttributeError on it now, so the assertion would have failed for a reason unrelated to what it asserts. `prop.style` is the API name; a volume actor has no `style` at all, and that difference is itself informative. coupling 309 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 469a80ce50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A third "specification, not method" subsection, matching 3.2 and 3.3. It is NOT filed under Limitations, and that distinction is load-bearing: a limitations section bounds work that was done, and none of this exists. Filed there it would assert a uranium model, a sink-term capacity, a damage scalar and a sensitivity constant that no file contains -- inverting the manuscript's discipline in the section least able to survive it. Uranium is not the modelled contaminant and the draft text asserting it would have been false. Repo-wide, uranium appears only in data/research/*.csv as REJECTED audit records -- both blocked, one stating "NOTHING WAS IRRADIATED: uranium is a chemical sorbate in this experiment" -- plus two Renslow citations in comments. biofilms_radiodialysis.R:161 is `c_ext = 1.0 # normalised`: no species, no Z. The text keeps the sorbate dimensionless and writes the high-Z argument conditionally, "for a high-Z sorbate such as uranium". No capacity figure is cited as reachable, since that would be a number with no measurement behind it. THREE SORPTION FORMS ARE IN PLAY AND THE TEXT SAYS WHICH IS WHICH. The implemented Eq. 20 already carries -(k_des + k_loss)s, so it was never the irreversible sink a naive k_ads*c*(1 - X/X_max) gives -- that form has a steady state only at X = X_max for any c > 0 and never uses an affinity constant. What the implemented form lacks is the CEILING: first-order in c, so its equilibrium is Henry, not Langmuir. Recovering Langmuir needs both terms and the ceiling. Also declared: an affinity constant is a RATIO and supplies no rate; the one-way coupling asserts d(mu)/dc = 0, which is a design choice and not a physical result; the resulting error reverses sign (loaded voxels under-estimated, shielded regions over-estimated) and for a heterogeneous model the reversal is the more consequential half; mu_matrix is not static either, since the CPM changes occupancy; spectral hardening under loading is BEAM hardening, an ordinary transport phenomenon, not an adaptive response; gamma in [0,1] can move D_eff only toward D_w, so a radiation-induced DECREASE in diffusivity is structurally inexpressible even though crosslinking competes with scission in polysaccharides -- a stronger restriction than monotonicity; permitting lambda -> 0 is not falsifiability; dose-time reciprocity spans about ten orders of magnitude between reactor Gy/min and environmental mGy/yr; and X_max = q_max * rho_dry inherits the weaker basis of its factors. The claim is enforced, not asserted. X_max, q_max, Langmuir, gamma_damage, lambda_rad and D_w are all absent from the simulation sources, and the test checks each -- with a planted-hit control first. It deliberately does NOT check k_ads, which really does exist in biofilms_radiodialysis.R: the prose says "no adsorption capacity", meaning the ceiling, and an over-broad absence test would fail against correct code. It also asserts k_des IS present, so the reversibility claim is checked rather than trusted. Verified by planting X_max in a source file: the test fails. PDF 28 pages, 0 overfull, 0 undefined refs, bibliography still 15 unused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
Two findings, both on fixes I made, and both are the same shape: the first pass closed the case in front of it and left the general one open. P1 -- ADDING biosafety_level_by_strain TO THE PLACEHOLDER LIST MADE IT REACHABLE, NOT VALID. That closed the fail-open on filler text; any other nonempty string still cleared it. "BSL2" -- a single level for a consortium whose strains sit at different levels, which is precisely what the schema warns against -- passed, and so did "DR:BSL1" against two declared strains, covering one organism while reporting all nine authorization criteria met. The field is parsed now: entries must be strain:level pairs, levels must be BSL1-4, keys must be distinct, and the count must match strain_identities. The convention is the producer's, not invented here: acquisition.py documents this field as "per-strain, e.g. 'SO:BSL1;CN:BSL2' -- never a single level". WHAT THE CHECK CANNOT DO IS ASSERTED RATHER THAN IMPLIED -- nothing declares how the key DR relates to "D. radiodurans R1", and inferring it from initials would be the consumer inventing semantics the producer never stated. So a mapping with the right count and wrong keys still passes, there is a test saying so, and that test fails if someone later adds real binding without correcting the docstring. P2 -- THE CONSOLE FIX COVERED ONE HELPER AND THE TEST EXERCISED ONLY THAT HELPER. main_coupled still printed absolute P_eff at two [RD] lines, and the Shiny status printed "P_eff(t_end)=%.4f cm/s" -- the fabricated unit the original finding was about -- so a reader running either program still received the withdrawn value by an ordinary path. All three now report P_eff/P0, dimensionless, which is what the manuscript reports. AND THE NEW SCAN'S FIRST VERSION WAS ITSELF BLIND. It filtered to lines containing printf/sprintf; in R the format string sits on its own line below the sprintf( call, so the offending line was never examined and the negative control passed against the reverted bug. A filter that narrows the search to where the author expected the defect is not a search. It scans every non-comment line now, and reverting either site is caught by file and line number. Controls: removing the mapping guard fails exactly the 7 refusal cases and leaves the 2 accept cases passing; reverting the R report and the Julia report are each caught. calibration 356, coupling 309/6 skipped, Julia 22+1 broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4021c0580
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if strains and len(pairs) != len(strains): | ||
| out.append( | ||
| f"biosafety_level_by_strain covers {len(pairs)} strain(s) and " |
There was a problem hiding this comment.
When the mapping has the correct number of pairs but names unrelated keys—for example, strain_identities="D. radiodurans R1; C. neoformans H99" with XX:BSL1;YY:BSL2—this count check emits no refusal, so after recomputing the scope hash all authorization criteria can report met without biosafety evidence for either declared strain. Fresh evidence beyond the prior review is test_the_mapping_check_does_not_claim_to_bind_keys_to_strain_names, which now explicitly codifies acceptance of this wrong-key case. Require canonical strain identifiers in both fields, or otherwise declare and validate an exact key-to-strain binding.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the second half of your comment is the part that matters — a test had codified the wrong-key case as acceptable. Fixed in 4d43131.
test_the_mapping_check_does_not_claim_to_bind_keys_to_strain_names asserted that XX:BSL1;YY:BSL2 passes, and the docstring it guarded explained why binding was impossible: nothing declared how DR related to D. radiodurans R1, so inferring it from initials would be the consumer inventing semantics the producer never stated. That reasoning is sound and the conclusion was wrong. Rule 4 says the producer declares semantics and the consumer must not assume them; it does not say the consumer records the gap and proceeds. What was missing was a declaration, not an inference.
So acquisition.py now makes one — the key is the strain identifier, verbatim — and there is no convention left to guess. approval.py compares the two fields as sets and reports both directions: a declared strain with no level, and a key naming an organism the row never declared. Those are different mistakes with different repairs. It replaces the count comparison and subsumes it; the omitted-strain case fails without it.
Matching collapses whitespace and ignores case. That is typography, not identity — looser about how a name is written, exactly as strict about which name it is.
One thing your comment did not name, found while fixing it: a strain identifier containing : cannot be a key, and without a guard it surfaced as "not a strain:level pair", sending the reader to fix the mapping — the only part of such a row that is right. Refused against strain_identities by name now.
Negative control: disabling the binding block fails four tests — your XX:BSL1;YY:BSL2 row, the abbreviation row, the omitted-strain row, and the dedicated binding test, which also asserts the well-formed fixture still passes so it cannot prove merely that something refuses everything.
D-APPROVAL stays outstanding. This makes the milestone harder to clear, not closer to cleared.
Leaving this thread open for your re-review rather than resolving it myself.
A printed leave-behind that opens on the bench protocol rather than the transport coupling: architecture, assay, data handling. The manuscript stays in the bag. THREE CORRECTIONS FROM THIS SESSION ARE BUILT IN, because the deck as first sketched would have reintroduced them. k_ads is NOT a Langmuir output. The isotherm yields q_max and b, and b is thermodynamic -- at equilibrium it is the RATIO k_ads/k_des, so infinitely many rate pairs share one value. Page 1 draws it as a dashed box outside the measured set, labelled "not supplied by an isotherm -- timed run, or a declared prior", and page 3 says the same in words. A deck that listed k_ads beside q_max as a bench deliverable would be promising a number the assay cannot produce. Orthogonality is a design choice, not a law. The deck says the codes are run in that order and that the dose field never reads concentration back, then states plainly that the assumption weakens at high loading. "Firewalled" would have claimed physics for what is really an execution order. Epistemic inheritance is on its own page. X_max = q_max * rho_dry multiplies a measurement by a prior, so the table types X_max as a PRIOR and says why -- a derived quantity cannot be stronger than its weakest input. Same rule data/calibration/suspended_isotherm_proposal.csv records. Page 2 carries what the requirement register already demands and the repo already knows: abiotic and handling-matched blanks, the fixed separation/duration/time-to-weighing that dominate reproducibility, and detectability-against-blank BEFORE the wet/dry CV before replicate sizing. Drying temperature is left as a question for her lab rather than proposed. Landscape letter, 3 pages, 0 overfull. The PDF is gitignored like the preprint's; the .tex is the source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
The golden-tally job has failed on every run since 06:21 UTC. It passed at
04:18. `7f05202` is authored 06:15. Six minutes.
That commit added an unpinned `vtk` to the new environment.yml, so conda-forge
resolved 9.7.0. Both workflows then run `pip install -e "coupling[dev]"` in
that env, whose extra carries `pyvista>=0.44`, and pyvista 0.48.4 requires
`vtk>=9.2.2,<9.7.0`. pip had to replace vtk and could not:
error: uninstall-no-record-file
x Cannot uninstall vtk 9.7.0
`-> The package's contents are unknown: no RECORD file was found for vtk.
A conda-built distribution ships no RECORD, so pip refuses to delete files it
has no manifest for. Reproduced in a throwaway env from this file before
changing anything -- the local pip is older and calls the same refusal
`uninstall-distutils-installed-package`.
I VERIFIED THAT ADDITION AGAINST TRANSPORT AND NOT AGAINST THE INSTALL. The
commit message argued at length that the solve moves nothing in the transport
path, re-ran all 12 OpenMC runs, and confirmed the fixture byte-identical --
all true, and all beside the point, because the job died three steps before
transport. The check I did not run is the one the next step performs.
It was never CI-only either. The local openmc-biofilms env carries the same
conda vtk 9.7.0, so the eight render tests 7519292 un-skipped could not be
installed there. And coupling-tests.yml's openmc-integration job builds the
same env and runs the same pip line; being workflow_dispatch-gated, it reports
"skipping" on every PR and carried the break in silence.
Naming `pyvista` hands the pair to one resolver. conda picks a vtk its own
pyvista accepts; pip finds `pyvista>=0.44` already satisfied and never reaches
for vtk. Pinning `vtk<9.7` here would work today and is pyvista's ceiling
copied into a second file by hand -- the fourth-copy drift this file exists to
end. Dry-run solve before committing: openmc, python, numpy, hdf5, libnetcdf,
moab, tempest-remap all unchanged; vtk 9.7.0 -> vtk-base 9.6.2, which carries
the Python module openmc's `mesh.write_data_to_vtk` imports; h5py build
102 -> 104 at the same 3.16.0, a bump the `vtk<9.7` candidate shows too and so
conda-forge drift rather than a consequence of this line.
AND THE TRIGGER STOPPED WATCHING THE SPEC (Codex P2, golden-tally:46). The
paths filter lists docs/openmc_stack.md because it used to hold the package
list; collapsing the copies moved that list out from under the filter. An
env-only change -- bumping the OpenMC pin -- matched neither trigger, and the
fixture would stay pinned to a stack nothing reran. That this PR's env change
was verified at all is luck: it touches other listed files.
`test_every_fixture_producing_module_triggers_verification` cannot catch it,
because it walks the regeneration script's imports and environment.yml is
data, not a module. Asserted in the single-spec test instead.
Negative controls, each run and each confirmed to fail before the fix:
* vtk 9.7.0 installed beside pyvista 0.48.4 -> the new test fails, naming
both versions and the specifier that excludes one.
* environment.yml removed from the paths filter -> single-spec test fails.
* environment.yml naming `vtk` again -> single-spec test fails.
MY FIRST VERSION OF THAT GUARD SKIPPED INSTEAD OF FAILING. It opened with
`pytest.importorskip("pyvista")`, and the first control run reported `1 skipped`
against the exact broken environment -- pyvista was installed and merely could
not import. That is rule 2 in the file that states rule 2. The check is
metadata-only and never needed the module; it now reads distributions and skips
only when one is genuinely absent.
The spec's package-name extraction split on `=` alone, so a `vtk<9.7` entry
would have yielded the literal `vtk<9.7` and failed the membership test for the
wrong reason -- reading as absent when present and merely pinned.
coupling 313 passed / 14 skipped (all OPENMC_CROSS_SECTIONS, no render skips),
calibration 351 / 5, contract 7. Verified in a fresh env built from the amended
file, where `pip install -e "coupling[dev]"` installs pyyaml and the editable
package and nothing else.
Codex's P1 on approval.py -- biosafety keys not bound to declared strains --
is NOT addressed here and stays open.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
Codex, twice, on the same field. The first round said adding
`biosafety_level_by_strain` to `_MUST_NAME_SOMETHING` made it reachable but not
valid; the fix for that added shape, level, distinctness and count checks. This
round says the count is not the binding, and it is right:
strain_identities = "D. radiodurans R1; C. neoformans H99"
biosafety_level_by_strain = "XX:BSL1;YY:BSL2"
Right shape, right count, distinct keys, two recognised levels. It passed, and
after recomputing the scope hash all nine authorization criteria reported met
carrying biosafety evidence for NEITHER declared organism.
A TEST CODIFIED THAT AS CORRECT. `test_the_mapping_check_does_not_claim_to_bind_
keys_to_strain_names` asserted the XX/YY row PASSES, and the docstring it
guarded explained why binding was impossible: nothing declared how `DR` related
to `D. radiodurans R1`, so inferring it from initials would have been the
consumer inventing semantics the producer never stated. That reasoning is sound
and its conclusion was wrong. Rule 4 says the producer declares semantics and
the consumer must not assume them -- it does not say the consumer records the
gap and proceeds. The missing piece was a declaration, not an inference.
So acquisition.py makes one: the key IS the strain identifier, verbatim. There
is now no convention to guess. approval.py compares the two fields as sets and
reports both directions -- a declared strain with no level, and a key naming an
organism the row never declared -- because those are different mistakes with
different repairs. This replaces the count comparison, which it subsumes: the
"fewer entries than declared strains" case fails without it.
Matching collapses whitespace and ignores case. That is typography, not
identity: it is looser about how a name is written and exactly as strict about
which name it is, and no normalisation here can turn one strain into another.
A strain identifier containing ':' cannot be a key, so it is refused against
`strain_identities` by name. Without that it surfaced as "not a strain:level
pair", sending the reader to fix the mapping -- the only part of such a row
that is right. A diagnosis pointed at the wrong field is its own defect.
Negative control, run and confirmed: disabling the binding block fails four
tests -- the XX/YY row, the abbreviation row, the omitted-strain row, and the
dedicated binding test, which also asserts the well-formed fixture still passes
so it cannot prove merely that something refuses everything.
The `DR:BSL1;CN:BSL2` fixture and the `'SO:BSL1;CN:BSL2'` example in
docs/calibration/reference_d_measurement_protocol.md were both abbreviations
and are now identifiers. Both shipped CSVs are header-only, so no data row
needed migrating.
calibration 360 passed, contract 7, coupling 310 passed / 6 skipped.
`D-APPROVAL` remains outstanding and nothing here approaches meeting it. This
makes the milestone harder to clear, not closer to cleared.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F82xpYUkxZpQhKuCGXgUjq
Resolve the sole .gitignore conflict additively: retain both the built-preprint exclusion and the geolocator cache exclusions. This imports the 13 master-only geolocator commits before collapsing the lower PR stack.
There was a problem hiding this comment.
🟡 Changes recommended
calibration/biofilm_calibration/approval.py mislabels structured refusal subjects for some biosafety mapping failures, undermining the stated “subject is data” contract used by downstream criteria mapping/reporting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR is a broad correctness-and-contract hardening pass across the Julia CPM, Python coupling/calibration harnesses, and CI workflows, aimed at eliminating “fail-open” checks and preventing expensive or misleading runs (especially where the expected state is “not ready”).
Changes:
- Adds negative-control-driven tests and gates to prevent silent drift (console report units, manuscript/code claims, merge preflight gate, OpenMC golden-tally verification, co-registration probes).
- Fixes ordering/validation issues so refusals happen before expensive work (e.g., scan path validation; subvoxel ratio validation; non-tiling rasterization pitches refused and explicitly recorded).
- Tightens viewer bundle semantics and rendering rules so absence/occupancy is producer-declared and cannot be silently guessed.
File summaries
| File | Description |
|---|---|
| tests/runtests.jl | Adds new Julia testsets to the suite. |
| tests/console_report_tests.jl | Asserts stdout reports don’t fabricate physical units. |
| tests/manuscript_claims_tests.jl | Adds manuscript/code/bibliography claim guards with negative controls. |
| scripts/preflight_merge.sh | Merge gate: blocks unresolved/stale findings (threads + comment-form). |
| scripts/test_preflight_merge.sh | Negative controls for merge gate, incl. pagination. |
| README.md | Updates docs to reflect fixed defects and current test counts. |
| Project.toml | Declares CairoMakie dependency. |
| preprint/wan_meeting_handout.tex | Adds meeting handout TeX artifact. |
| environment.yml | Introduces single pinned OpenMC transport env spec. |
| docs/openmc_stack.md | Switches env creation to environment.yml; expands fixture guidance. |
| docs/preprint_revision_plan.md | Updates status/notes for fixed items and remaining defects. |
| docs/jacc_coupling_port.md | Notes Metal backend Float32 boundary requirement. |
| docs/exchange_schema.md | Clarifies lattice vs mesh tally transform differences; adds probe rationale. |
| docs/calibration/reference_d_measurement_protocol.md | Tightens per-strain biosafety mapping semantics. |
| docs/calibration/openmc_nested_pilot_report.md | Documents publish-blocker for gate/significance contradictions. |
| docs/calibration/morphology_rasterization_ladder.md | Withdraws non-tiling pitch row; documents corrected finding. |
| docs/research/session_claims_2026-08-24_redteam.md | Records red-team assessment of unsourced session claims. |
| docs/research/murr_facility_candidate.md | Adds forward-looking, explicitly unsourced venue note. |
| data/species_parameter_provenance.csv | Adds reshaped Table 2 provenance data for querying. |
| data/calibration/suspended_isotherm_proposal.csv | Adds non-authoritative planning worksheet for isotherm work. |
| data/calibration/spatial/sources.csv | Adds new source registry rows (abstract-level, needs verification). |
| data/calibration/reference_d_requirements.csv | Fixes documented status vocabulary to match enforced code. |
| coupling/pyproject.toml | Moves PyVista + PyYAML into dev extras to avoid skip-hidden defects. |
| coupling/biofilm_openmc/drivers.py | Ensures scan validates configs before calling runner. |
| coupling/biofilm_openmc/synthetic_gate.py | Refuses <2 draws to avoid “PASS” on df=0. |
| coupling/biofilm_openmc/viewer.py | Adds background/occupancy declarations and stricter bundle validation. |
| coupling/biofilm_openmc/observer.py | Fixes categorical rendering semantics and species palette correctness. |
| coupling/tests/test_viewer_bundle.py | Updates Layer background declaration in tests. |
| coupling/tests/test_synthetic_gate_fixtures.py | Adds negative control for single-draw refusal. |
| coupling/tests/test_mesh.py | Adds spy-based test ensuring scan refuses before runner runs. |
| coupling/tests/test_grid_coregistration.py | Adds world-space co-registration probes + negative controls. |
| coupling/tests/fixtures/pilot_verdict_significance.json | Adds committed fixture for publication-block checks. |
| coupling/scripts/synthetic_e2e.py | Writes multi-grid viewer bundle from e2e artifacts. |
| coupling/scripts/subvoxel_refinement.py | Refuses non-divisor ratios before heavy imports/transport. |
| coupling/scripts/regenerate_golden_tally.py | Adds script to regenerate golden tally fixture deterministically. |
| calibration/biofilm_calibration/spatial/synthetic.py | Refuses non-tiling pitches via NonTilingPitchError. |
| calibration/scripts/rasterization_ladder.py | Records refused pitches and prevents skipped rungs breaking tails. |
| calibration/tests/test_rasterization_ladder.py | Adds tiling/refusal tests and default-ladder reachability tests. |
| calibration/biofilm_calibration/approval.py | Adds structured refusal subjects and stricter biosafety mapping checks. |
| calibration/biofilm_calibration/acquisition.py | Documents biosafety mapping keyed by strain IDs verbatim. |
| calibration/scripts/reference_d_status.py | Fixes authorization milestone reachability and mapping robustness. |
| calibration/tests/test_approval.py | Expands approval negative controls and mapping assertions. |
| calibration/tests/test_reference_d_requirements.py | Adds reachability + mapping-negative-control tests for authorization criteria. |
| biofilms_potts.jl | Makes membrane report honest; factors report into testable function. |
| biofilms_radiodialysis.R | Replaces absolute permeability print with dimensionless ratio. |
| AGENTS.md | Adds repo-wide six-rule standards document. |
| CLAUDE.md | Adds agent-specific guidance pointing to AGENTS.md. |
| .gitignore | Ignores built preprint PDFs. |
| .github/workflows/coupling-tests.yml | Adds xvfb render support; adds merge-gate job; uses env spec file. |
| .github/workflows/golden-tally-verification.yml | Adds real OpenMC+data verification tier with correct triggers. |
Review details
- Files reviewed: 55/58 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for text in _biosafety_mapping_problems(row): | ||
| add("biosafety_level_by_strain", text) |
Codex reviewed PRs #7–#10 after they were merged. I never went back for those comments. Every issue here was live on
master.The two that matter most — both in refusals I described as working
scan()ran transport before validating. I wrote that it "refuses up front." It did not:sp = runner(build(cfg), name)executed beforeper_source_from_statepoint(sp, mass_of(cfg)), so a misconfigured ladder burned the entire sweep and then refused. Validation now precedes the runner.The authorization milestone could never pass.
authorization_criteria(baseline)omittedsources, leaving criterion 8 — approval provenance recorded and retrievable — permanently unmet. Demonstrated before the fix:and after, with the same registry:
0 unmet.This is the worst failure shape in the repository. An unreachable milestone is indistinguishable from a correctly withheld one, because "not authorized" is the expected state. Nothing would ever have looked wrong.
The one that changes a published number
3.2 µm does not tile the spheres' 24 µm axis — 7.5 voxels — and
_grid_shaperounded to 8. That rung rasterised a 48×48×25.6 µm box while the closed-form truth kept dividing by the declared 48×48×24. A 6.7% denominator shift was being reported as rasterisation error, in the one study whose entire purpose is to hold the object fixed and move only the sampling.Non-tiling pitches now raise
NonTilingPitchError; the ladder names the pitch it skipped instead of silently dropping the row.Only component size moves — it was passing at a pitch that never validly ran. The interface-area finding was measured at pitches that tile and stands unchanged. Recorded as ledger row
LADDER-04and as a marked correction in the Phase E report.The rest
approved_protocol_versionis a scope column, so a blank value hashed as blank and the digest still matched — an approval could be recorded without naming which protocol the institution reviewed. Added to_MUST_NAME_SOMETHING.--publishpublished results from a run thatstopped_early._t_critical()extrapolated below df 1; now returnsNonethere, and a conservative tabulated value otherwise.observer.plot_layervolume-rendered categorical fields.Verification
coupling 214 passed / 8 skipped · calibration 298 · contract 7 · Julia 9.
New tests are negative controls: the tiling test asserts the pitches that do tile reproduce the declared extent exactly, and the approval test asserts a blank protocol version is refused.
Reference D stays
NOT_EVALUATED,CAMPAIGN_READYstays no, δ stays unset. The authorization milestone is now reachable — it is not met, and nothing here approaches meeting it.🤖 Generated with Claude Code