Reconcile bpsi/develop base into develop (bpsi↔fork merge baseline, CI-green) - #226
Conversation
For web-client EQ→TR file coupling. Adds ~25 lines to eq_api.f90. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Exposes the eq_save C-ABI symbol (added in Task 1.1 / SHA eb55843) as Eq.save(path): sets KNAMEQ then calls eq_save, raising EqlibError on non-zero rc. Adds ctypes prototype in _ffi.py and a TDD integration test that verifies file existence and non-zero size. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add handle_save handler and @mcp.tool() save registration; update --print-tools list and the test_print_tools_output count from 11 → 12. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… channel Fortran's WRITE(6,...) maps to OS fd 1 (process stdout), which is the same fd the MCP stdio transport uses for JSON-RPC framing. Any Fortran diagnostic line written to fd 1 corrupts the channel and causes 'Connection closed'. Add _redirect_fortran_stdout_to_stderr() context manager to eq_mcp and tr_mcp servers: saves fd 1, dup2s fd 2 (stderr) over fd 1 before Fortran calls, then restores fd 1 afterwards. Applied to eq.run(), eq.save(), and tr.run(). Do NOT use fflush(NULL): on macOS/Python it also flushes the Python-level asyncio write buffer while fd 1 still points to stderr, sending the pending JSON response to stderr and causing Connection closed on the client side. Fortran WRITE(6,...) is line-buffered, so output reaches stderr before fd 1 is restored without an explicit flush. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previous redirect protected fd 1 during the Fortran call but didn't flush libc's stdout buffer. Pending Fortran output then leaked into the MCP pipe after restoration, corrupting JSON-RPC on subsequent calls (notably get_state after run). Now we fflush() libc's C-level stdout FILE* before restoring fd 1; this does NOT affect Python's asyncio writes because Python writes via syscalls, not via C stdout. Also extend the redirect to cover set_param, set_param_str, set_params, get_state, and finalize in both eq_mcp and tr_mcp, since those handlers also call Fortran routines that may print.
Add setvbuf(stdout, NULL, _IONBF, 0) at server startup in both eq_mcp and tr_mcp so that any Fortran WRITE(6,...) output is emitted immediately rather than staying in libc's buffer after the fd-level redirect has been torn down. Also add _gfortran_flush_i4 (Fortran-level I/O buffer flush for unit 6) inside _redirect_fortran_stdout_to_stderr() so that gfortran's own internal buffer is drained into stderr before fd 1 is restored to the JSON-RPC pipe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…imi#194) Adds belt-and-suspenders fd-isolation at module load time, beyond the per-call redirect already in 69d3d9d4. Disables _gfortran_flush_i4 in the long-lived server path to avoid SIGABRT during teardown. Uses os._exit(0) in main() to bypass Python finalizers that race with Fortran cleanup. Local-only patch on kyoshimi-develop; will be re-evaluated after upstream macOS build support (f8f5c9e) lands and macos_stubs.f90 is dropped (which may make the SIGABRT-on-teardown class obsolete).
|
Careful review completed. I found three correctness issues that should be addressed before merge:
I opened #227 with reproduction details, required fixes, and regression-test expectations, and assigned it to @HengyuLi-Ozaki-lab. Please also run the required GitHub Actions against the updated PR head. At review time, this PR reports no checks for fb80991. Review outcome: changes requested pending #227. |
|
Non-MCP follow-up review is complete. It found additional merge-blocking regressions outside the MCP servers, including:
I opened a separate follow-up issue for the full evidence, affected lines, verification results, and acceptance criteria: #228 Recommendation remains do not merge yet. This issue is separate from the MCP findings tracked in #227. |
|
A further non-MCP audit found six additional defects, including three new P1 merge blockers:
The additional P2/P3 findings cover FPX debug output inside deep NBI loops, WF2DT use of deallocated RGB arrays, and validation of the wrong WIM record index. Full evidence, reproduction output, affected lines, and acceptance criteria were added to #228: The recommendation remains do not merge yet pending the P1 fixes. |
|
Clavius runtime verification found three additional P1 merge blockers and one regression-harness defect at the current PR head
WIM wave-dump generation/conversion and changed W1, WM/WMX, WR/WRX, FP, and OB library builds succeeded on clavius. WF2DT end-to-end plotting remains unverified because all committed GLAST inputs stop at the antenna setup error. Full reproduction details and acceptance criteria: Recommendation remains do not merge yet until the P1 items and regression contract are resolved. |
|
An independent re-audit at the current head
Details added to #228 (findings 18-19): #228 (comment) Additionally, five defects in the Positive verification results: |
|
A fifth review wave audited defect classes not covered by the earlier findings: statement-level verification of the equ/ fixed→free-form conversion against the original sources, a repository-wide impact sweep of the newly introduced shared symbols and moved APIs (including unmodified components), and CI-workflow logic. It found four additional P1 merge blockers, nine P2 defects, and ~25 smaller issues at the current PR head
Notable P2s: the plasmaf-overflow this PR fixes in Full evidence, line references, and acceptance criteria:
Recommendation remains do not merge yet until the P1 items across #227/#228 are resolved and the workflows run green on the PR head. |
|
A fresh re-review of the unchanged head The PR moves Fresh clavius results: Both runs calculate and reach Full evidence and acceptance criteria were added as finding 32: The same re-review also reconfirmed the existing EQ regression failures, TRX input error, TXNEW |
|
Thank you both — five review waves plus an independent re-audit, including real runs on clavius, is far more scrutiny than this PR had any right to expect, and it found real problems. I've now worked through every finding in #227/#228/#229 and re-verified each one against Below: what's ours and being fixed, one thing I need from a maintainer, two corrections with reproductions, and a proposal for how to land this. 1. What I need from a maintainer (this is the blocker)The workflows are not failing — they were never allowed to start. Zero jobs were dispatched. This also gates the EQ baseline regeneration you asked for (§4): 2. Confirmed ours — being fixedFour P1s are unambiguously mine, and one methodology failure that I think is worse than any of them. 2.1 2.2 MCP import mutates process-wide stdout (#227 item 1). Confirmed. 2.3 tr_mcp lifecycle is self-contradictory (#227 item 2). Confirmed. 2.4 2.5 The EQ baseline gap — you're right, and my justification was invalid. The decision record for the One correction to the root-cause attribution, though: it isn't Plan: regenerate 2.6 One defect nobody found: 2.7 CI never compiles 3. Two corrections, with reproductions3.1 The The mechanism claim doesn't reproduce. Same result on macOS/BSD There is a small real residue nobody flagged: on a repo literally named 3.2 There's a hard check: Suggest closing that item as not-a-defect. (The Fortran-side 80-char limit is real and undocumented at the C ABI — worth a doc note, but nothing is silently truncated.) 4. Proposed de-scoping (evidence for each)
For the rest: of 162 changed files, 94 are byte-identical to bpsi My proposed split for those: fix here anything that breaks the build or is unguarded UB on a default path — findings 1, 5, 8, 9, 10+15, 12, 14, 20, 21, 23, 24 (~9 small changes; note 10 and 15 must land together, since routing 5. How I'd like to land thisCopilot declined this diff for exceeding 20,000 lines, and that's a fair signal. But I don't think splitting #226 helps: the bulk is Instead I propose buying review confidence with CI rather than with smaller diffs:
If you'd rather I fold everything into #226 instead, I'm happy to — just say which you prefer. Sorry for the volume of work this created. The |
…not deallocate Addresses k-yoshimi#228 finding 32 (P1, integrated-lifecycle regression). The merge landed bpsi's ownership move of DEALLOCATE_TRCOMM from the standalone program into the reusable menu. bpsi can do that safely because bpsi has no integrated TOT regression path (`git ls-tree -r fa9dd49 tot/` = 12 files, no totregress.f90; k-yoshimi's tree has 29 and does). On this tree, tot/totmenu.f90:48 calls tr_menu, so exiting the TR submenu destroyed RN/RT/AJ/QP before TOT could read them. Reproduced locally before the fix, on tot_demo2014_short (the input enters `tr`, runs, then `x` exits the submenu — exactly the 9000 path): TOT_REGRESS_DUMP=1 tot/tot < test_run/inputs/tot_demo2014_short.in -> TR_PRESENT=0 (matches the reviewer's clavius result) After: -> TR_PRESENT=1, NT=100, NRMAX=50, NSMAX=2, WPT=1.6203558665082904E+00 Fix: drop the call from tr/trmenu.f90's 9000 handler and restore it in tr/trmain.f90 before STOP, i.e. back to the arrangement develop already had. Cleanup stays owned at the other two layers too — tr/tr_api.f90 (tr_finalize) for the library/MCP path, and tr/trcomm.f90's ALLOCATE_TRCOMM is self-healing on re-entry — so removing the menu-level call leaks nothing. Verified the standalone path still runs clean (tr2 on tr_m0904: exit 0, full dump, no double-free). The USE list keeps only GTCPU1 + DEALLOCATE_TRCOMM; NFM/NGM/NSM/NTM are not referenced anywhere in trmain.f90 (grep confirms zero hits).
Addresses k-yoshimi#227 item 1 (P1). eq_mcp/server.py and tr_mcp/server.py performed the JSON-RPC fd-isolation dance -- dup(1), dup2(2, 1), rebuild sys.stdout -- at *module scope*. Importing either module therefore mutated the host process: pytest collection, an embedding application, or a bare `python -c "import eq_mcp.server"` had its fd 1 redirected to stderr and its sys.stdout replaced, irreversibly and process-wide. The shim now lives in `_install_fd_isolation()` (idempotent, sets _FD_ISOLATION_INSTALLED) and is called from main() immediately before `build_server()` / `server.run()` -- i.e. only when the module actually runs as the stdio server. The --help / --print-tools / MCP-SDK-missing paths all return before that point, so the previous `_is_oneshot` guard is no longer needed and the dead `_ONESHOT_FLAGS`/`_is_oneshot` pair is removed. One knock-on: `_HAS_GFORTRAN_FLUSH` was forced False at import time whenever the permanent redirect was active (the flush is unnecessary once fd 1 is stderr, and _gfortran_flush_i4 SIGABRTs 10-40% of the time on this libgfortran). That decision can no longer be made at import time, so it moved to the call site in `_redirect_fortran_stdout_to_stderr`: `if _HAS_GFORTRAN_FLUSH and not _FD_ISOLATION_INSTALLED`. Same behaviour for the server, correct behaviour for an importing process. Verification: - New regression test python/mcp-servers/eq_mcp/tests/test_import_isolation.py probes both modules in fresh subprocesses. It PASSES with this change and FAILS on the pre-fix code (checked by stashing), so it genuinely pins the contract rather than just passing. before: fd1_unchanged=False sys.stdout_unchanged=False (both modules) after : fd1_unchanged=True sys.stdout_unchanged=True (both modules) - The real stdio path is unaffected: with eq/libeqapi.so built, test_get_psi_rz_tool.py::test_get_psi_rz_via_mcp -- which spawns the server as a subprocess and speaks JSON-RPC over the pipe -- passes. That test is the one that would break if the isolation were lost. - Full suites: eq_mcp + tr_mcp = 77 passed, 3 skipped, 0 failed (--forked). (Without a built libeqapi.so the psi_rz test fails on both the pre- and post-fix code with a JSONDecodeError -- a missing-.so artifact, not a regression.)
Addresses k-yoshimi#227 item 2 (P1). handle_finalize marked the Trlib handle closed *without* calling tr_finalize, to dodge a SIGABRT in the Fortran cleanup path. Two consequences, both real: - tr_init is idempotent (tr/tr_api.f90:77 "already initialized, just return OK"), so leaving g_initialized set made a subsequent init a no-op that did not restore defaults. Verified: set NSMAX=3, finalize, init -> still 3. - handle_run_and_get_state needs a genuine reset for its documented fresh-init contract, so it kept calling STATE.close() -- the very tr_finalize path handle_finalize was avoiding. The server contradicted itself, and eq_mcp (plain STATE.close()) disagreed with tr_mcp. The SIGABRT it was working around is a double DEALLOCATE, and it is already fixed on this tree: DEALLOCATE_TRCOMM gained an idempotency guard (`IF(.NOT.ALLOCATED(PNSS)) RETURN`, tr/trcomm.f90) that develop does not have -- `git show 4a5d977:tr/trcomm.f90` has the unguarded body. tr/tr_api.f90 itself is unchanged vs develop, so the fix is entirely in the deallocator. Measured on the merged tree (gfortran-15, macOS, fresh subprocess per run): init + close 40/40 exit 0 init + run(1) + close 40/40 exit 0 init + run + close + an explicit second tr_finalize 30/30 exit 0 (2nd returns TR_OK via the not-initialized guard) So both lifecycle paths now use the honest one: handle_finalize is `with _redirect_fortran_stdout_to_stderr(): STATE.close()`, structurally identical to eq_mcp's. Contract restored -- set NSMAX=3, finalize, init -> 2. New tests in python/mcp-servers/tr_mcp/tests/test_lifecycle.py cover finalize->init reset, finalize idempotency, and run_and_get_state isolation. The reset test FAILS on the pre-fix code (checked by stashing) and passes now. Full eq_mcp + tr_mcp suites: 83 passed, 0 failed. Caveat worth stating: the crash-rate measurements are macOS/gfortran-15. The authoritative check is this PR's Linux/gfortran-13.2 CI, which is still awaiting the fork-PR workflow approval.
Addresses k-yoshimi#227 item 3 (P1). eqfile::EQSAVE is a bare external subroutine with no IERR out-argument: on an FWOPEN failure (blank KNAMEQ, missing directory, permission denied) it prints a message and RETURNs, leaving no file. eq_api_save then returned EQ_OK unconditionally. Our own docstrings admitted this on both sides -- eq_api.f90 said "EQ_OK is returned unconditionally ... The caller is expected to verify the file's existence", and eqlib.py said "callers should verify file existence after the call". Shipping a known false-success with a comment instead of a guard was the wrong call. Fortran layer (eq/eq_api.f90) -- verify the artefact rather than trust the call: - blank KNAMEQ -> EQ_ERR_INVALID - no file, or a zero-length file -> EQ_ERR_CALC_FAILED - otherwise -> EQ_OK EQSAVE's own signature is untouched, so eqmenu.f90:98 (`CALL EQSAVE`, the interactive path) is unaffected. Python layer (python/eqlib/eqlib.py) -- the same check after raise_for_rc, as defense in depth so a stale libeqapi.so built before this fix also cannot report a success that did not happen. Both docstrings rewritten to state the contract instead of apologising for its absence. Verified against the rebuilt .so: missing directory -> EqlibCalculationFailedError (rc=3) blank path -> EqlibInvalidParamError (rc=1) valid path -> OK, 45956-byte file written Control (pre-fix eq_api.f90 + eqlib.py restored, .so rebuilt): save() to a missing directory returned SUCCESS -- exactly the reported defect. New regression tests in python/eqlib/tests/test_save.py cover the missing directory and blank-path cases. Suites: eqlib + eq_mcp + tr_mcp = 133 passed, 2 skipped, 0 failed.
… 6 more components Addresses k-yoshimi#228 findings 1, 20, 21 (P1). The merge carries two bpsi changes to shared pl state: - plcomm_parm gained a bare PT(NSM) (pl/plcomm.f90:66). develop has no PT in that declaration block at all -- verified with `git show 4a5d977:pl/plcomm.f90`. - pl_view moved plparm -> the new pl/plview.f90 (develop's plparm has 3 refs; head has 0). Our earlier CI-greening commits (17f21f2, 9242c12) contained this for ti, wm, wmx and four menus, but only for the components CI builds. CI never compiles trm/ wmf/ wmfn/ wmseki/ dpseki/, so the rest went unnoticed. All six victim files are byte-identical to develop -- they are untouched casualties of a change we are the courier for. Reproduced before fixing (`make -C <dir>` from a clean worktree): trm Error: Symbol 'pt' conflicts with symbol from module 'plcomm_parm' wmf Error: Incompatible ranks 0 and 1 in assignment (wmfem_vmec.f:517) wmfn Error: Incompatible ranks 0 and 1 in assignment wmseki Error: Incompatible ranks 0 and 1 in assignment dpseki Error: Symbol 'pl_view' referenced at (1) not found in module 'plparm' Fixes use the pattern the earlier commits established: - `USE plcomm,pt_pl=>PT` / `USE dpcomm,pt_dp=>PT` in trm, wmf, wmfn, wmseki. wmf needed BOTH routes (it imports plcomm and dpcomm; dpcomm_parm re-exports plcomm_parm's PT) -- the same double rename wmx/wmcomm.f90:11,24 already has. This restores the pre-merge meaning exactly: before bpsi added PT to plcomm_parm, bare PT in these legacy fixed-form files was an implicit local scalar, which is what the rename gives back. - `USE plview,ONLY: pl_view` in dpseki/dpmenu.f and dpseki/dpcont.f (the latter split from a combined `USE plparm,ONLY: pl_parm,pl_view`). Separately, wm/wmsetg.f:310-311 is the silent variant of the same defect and is the more dangerous one: `PT=(PTPR(1)+2*PTPP(1))/3.D0` is a legal Fortran broadcast that overwrites the WHOLE shared PT(NSM) array with the electron temperature, and `PT(1)` then reads it back. It compiles cleanly, so CI could never have caught it -- wm builds green today. Changed to the local scalar PTAV, matching the correct sibling at wm/wmsetg.f:185-186 (and wmvmec.f:530, wmbooz.f:570). After: trm, wmf, wmfn, wm, wmx, ti all compile. dpseki compiles (its remaining link failure is a missing ../mtxp/libmtxbndmpi.o, an MPI artifact not built in this worktree, not a code defect). Two things found while doing this that are NOT ours, for the de-scope list: - fpx fails on fpcomm.f90:621 `IF(ALLOCATED(SAVPOS))` where SAVPOS is not declared ALLOCATABLE in that scope. fpx/fpcomm.f90 is byte-identical to develop, so fpx has never compiled -- pre-existing, unrelated to plcomm. - wmseki, after the PT fix, next fails on 'pl_wmxprf'/'wmspl_prof' not found in module plprof. pl/plprof.f90 is byte-identical to develop -- also pre-existing. wmseki additionally has no mod/ directory in the repo while 55 other mod/ files are tracked; that packaging gap is left alone here. Full python suite: 601 passed, 169 skipped, 0 failed.
…den its fallback Addresses k-yoshimi#228 findings 10 and 15 (P1). They must land together -- see below. Finding 10: the new MDLPCK=2 block-tridiagonal solver calls raw DGETRF/DGETRS (7 sites, txnew/txexec.f90:1197-1271). Nothing in the tree defines those symbols, and the repository default is a LAPACK-free build (make.header.org:13-14 `LAPACK = nolapack.f`, `LIBLA =`), so `make -C txnew` cannot link at all. Every other solver in the tree goes through the LAPACK_* shim in lib/ instead -- which had wrappers for DGBTRF/DGBTRS/DGBSV/DSBTRD/DSTEBZ/DGESV but none for DGETRF/DGETRS. Added the two missing wrappers to lib/lapack.f (real calls) and lib/nolapack.f (INFO=-1 stubs, matching the file's existing style), and pointed the 7 call sites at them. Finding 15: fixing 10 alone would have converted a loud link error into a silent wrong answer. TXSOLV_BLOCKTRI maps a LAPACK failure to `IER = 200000/220000 + 1000*k + info` (txexec.f90:1199,1212,1222,1229) -- always POSITIVE -- while the caller's fallback fired only on `ierr_la == -2` (txexec.f90:237, the "BL has non-zero far blocks" case). So with the nolapack stubs returning INFO=-1, IER lands at 200000+1000k-1, the fallback is skipped, and control drops into the error branch with the solution vector never solved. The fallback now fires on any non-zero ierr_la, so a factorization failure -- from the stubs or from a genuine numerical problem -- degrades to the band solver (LAPACK_DGBSV/GBSV) that the rest of the file already relies on. If that also fails, the existing `if(ierr_la /= 0)` error branch still catches it. Provenance: txnew/txexec.f90 is byte-identical to Kyoto bpsi/develop (fa9dd49); develop (4a5d977) has no raw LAPACK calls in txnew at all. This is Kyoto's code that our merge is the courier for, but it breaks the default build, so it is fixed here rather than deferred. Verified: - `make -C txnew libtx3.a` -> exit 0 (was: "Undefined symbols ... _dgetrf_"). - `nm -gU lib/libtask.a` shows _lapack_dgetrf_ / _lapack_dgetrs_ in BOTH the real-LAPACK build and a `LAPACK=nolapack.f` rebuild. - No undefined _dgetrf_/_dgetrs_ remain in the txnew objects.
Addresses k-yoshimi#228 findings 5, 8, 9, 12, 14, 18, 19, 23, 24. All ten sites are byte-identical to Kyoto bpsi/develop (fa9dd49) -- this merge is their courier, not their author -- and each was independently reproduced before patching. equ/ [9, P1] equ_convert.f90:42 `DO n=n,LEN(line)` reads n uninitialised as its own loop bound. Reproduced: gfortran -Wuninitialized flags it; the same extract with `DO n=1,` compiles clean. Garbage > 256 => zero commas => ALLOCATE(...(0)) then a read of element 1; garbage < 1 => out-of-bounds substring. Bound is 1, not 5: the record is `hdr,L1,...,Ln` with the header's comma at column 4 and no trailing comma, so counting from column 1 yields exactly ndata commas for ndata labels. [18] equpl.f90:91 restores `plasmaf%nrmax=0`, dropped by the F77->F90 conversion (`git show 7d18264:equ/eqpl.f`:91) while the sibling eqpl_get kept it at :163. NOTE FOR THE REVIEWER: this is symmetry / F77 fidelity, NOT the claimed P1. Against the bpsd this build pins (../bpsd @ 6a85985), bpsd_get_plasmaf takes its argument intent(out) and assigns nrmax unconditionally, so the store is provably dead -- demonstrated with a standalone intent(out) test. It was load-bearing under an older intent(inout) bpsd, which is why the line existed. wf2dt/ [8, P1] wfgsubn.f90: the PABS plot wrote rnode into BOTH coordinates (every triangle drawn at y=R, collapsing the mesh) and passed a 3-element vertex array to poly2D(...,4). Elements are triangles (ndelm(1:3,...)), and the fill loop above is DO i=1,3, so 3 is the true count and the 4th read was pure garbage. Now ya <- znode and poly2D(...,3). [12+31] wfglib.f90: rgbf_a/rgbf_b DEALLOCATEd rgb_a/rgb_b and then read them in the clamp branches below (f < f_a(1) fires on any negative pabs) -- rgbf_c already had this right and frees only the scratch array. Same patch also unwinds all three SPL1D failure paths, which previously RETURNed with the arrays allocated and INIT still 0, so the next call re-ALLOCATEd allocated arrays and aborted. [19] wfglib.f90:567-568 `1.-12` parses as -11.0, so `ABS(fmax).LE.-11.0` is never true and the near-zero colour-range fallback never fired. Now 1.D-12. (Provenance note: terasakisatoshi filed this as PR-introduced; it is byte-identical to fa9dd49, i.e. carried, not authored here.) trx/ [5, P1] trpnb.f90:477 zero-initialises VCD3/VCT3/VCA3 before the three IF(NS_x.NE.0) guards. develop assigns all three unconditionally; the new guards leave a term undefined when a species is absent, and VC3 = VCD3+VCT3+VCA3 is then used as a divisor. [24] trprep.f90:131 `DO NS=1,MAX(NSMAX,4)` -> `DO NS=1,NSMAX`, restoring develop's bound. Labelling species past NSMAX defeats libnf's presence STOP and enables out-of-bounds writes in tr_pnf. Applied TOGETHER with finding 5 by design: reverting the bound without the zero-init would widen finding 5's exposure. [23] trexec.f90: the new "Kawamura correction" dropped the NSTN.EQ.0 guard every sibling branch has, reading X(0) when the equation is excluded. Restored, with develop's pre-Kawamura formula in the NSTN==0 arm. [14, P1] in/test01.in, in/test02.in: `model_nnf(1)=4` is not in NAMELIST /TR/, so every shipped run printed '## PARM INPUT ERROR' and then continued with an unintended configuration. The variable is a late-allocated ALLOCATABLE outside the namelist scope and no physics reads it, so removing the key is the correct fix rather than extending the namelist. Verified from trx/ (the cwd the binary needs -- running elsewhere makes GSFOPN fail and the menu never reaches the namelist read, which invalidated my first attempt at this check): before: PARM INPUT ERROR = 1 on both inputs after : PARM INPUT ERROR = 0 on both inputs Verification: equ, trx, wf2dt, trm, wm, wmx, wmf, wmfn, tot, lib, pl, eq, tr all compile. Python suite 601 passed / 0 failed. TOT still TR_PRESENT=1. The tr_m0904 TR_REGRESS dump is byte-identical to the pre-refactor baseline (sha256 56d09887...cb9ad869), i.e. none of this perturbs tr numerics. Separate observation for the PR thread, NOT patched here: develop's trx/in/test01.in set `model_pnf=1`; bpsi's copy does not, and the merge took bpsi's file, so the shipped sample no longer exercises the fusion model at all. That is a test-coverage regression worth a maintainer decision rather than a unilateral edit.
… step Two CI changes, both prompted by PR k-yoshimi#226's review. 1. New `uncovered-modules-build` job (compile-only). The pytest job builds tr/fp/ti/wr/wrx/eq/tot, and `make -C tot libs` adds pl/dp/wmx/ob/adf11/adpost. equ/ trx/ txnew/ wf2dt/ wim/ wm/ trm/ wmf/ wmfn/ have had NO CI coverage at all -- and every one of them is substantially touched by the bpsi merge. That gap is the structural reason a batch of hard compile/link breakages reached human review in k-yoshimi#226 instead of the pipeline: a bare PT added to plcomm_parm colliding with local PTs (trm, wmf, wmfn, wmseki), pl_view moving plparm->plview without migrating its callers (dpseki), and raw DGETRF/DGETRS with no LAPACK shim (txnew). Nine directories, library targets only -- the standalone binaries pull in graphics (gsaf) and MPI link paths this headless runner deliberately does not provide, while compiling every source is what catches the defect class above. Deliberately excluded, with the reason recorded in the job comment so the omission is auditable rather than silent: fpx, wmseki and dpseki do not build on `develop` either (their failing files are byte-identical there), so including them would make the job red on day one for defects this PR did not introduce. All nine targets verified green locally before wiring them up. 2. Path-equality guard on the `../task` symlink step (both workflows). The step's own comment claimed it was a "no-op when the repo is already named task". It is not: when the destination is a real directory, `ln -sfn` falls back to link-into-directory mode and creates a self-referential `task -> .` symlink INSIDE the checkout. Reproduced on GNU coreutils 8.32 (the CI toolchain) and on BSD ln -- both exit 0, so it is silent. Harmless for the current job graph, but a symlink loop for any future recursive find/tar/artifact-glob step. Now guarded on path equality, and both stale comments corrected. Note for the record: this is NOT why PR k-yoshimi#226 reports no checks. That run (29195792410) is `conclusion: action_required` with `{"total_count": 0, "jobs": []}` -- the fork-PR approval gate, so no step ran at all. regen-baselines.yml is additionally `workflow_dispatch`-only and can never contribute a PR check.
…atch
Two independent reviews found two blockers and several real gaps. Both blockers
were in work I had already called verified, so the corrections are recorded here
rather than folded silently into the earlier commits.
BLOCKER 1 (channel A) — the new CI job could not have passed.
My "all nine targets verified green locally" was a dirty-worktree false green:
this worktree carries untracked trlib/mod/*.mod and equ objects from earlier
builds. Re-tested properly in a PRISTINE clone of HEAD, 3 of 9 failed:
trx, trm Cannot open module file 'cytran_mod.mod'
equ Cannot open module file 'aaa_mod.mod'
Root causes, both real Makefile facts rather than environment:
- trx/ and trm/ compile against trlib's .mod files via `-I../trlib/$(MOD)`,
and neither libtr2.a target bootstraps trlib. Only trlib/mod/dummy is
tracked, so a fresh clone has none of them.
- equ/'s `libequ.a: $(OBJS)` covers SRCS only, but equcomm.f90 -- which
defines aaa_mod, USEd across SRCS -- lives in the separate SRCC/OBJC
variable, so the library target alone can never build from clean.
Added `make -C trlib libtrm.a` and `make -C equ obj/equcomm.o` to the support
step. Re-verified in a pristine clone: 9/9 green, job exits 0.
BLOCKER 2 (channel B) — k-yoshimi#228 finding 21 was only 2 of 6 sites.
b4c07ef migrated dpseki/dpmenu.f and dpseki/dpcont.f but missed
fpx/fpmenu.f90:23, fp.anzai/fpmenu.f90:19, fp.nuga/fpmenu.f90:19 and
fp.ota/fpmenu.f90:19 -- all four still `USE plparm` and `CALL pl_view`, and
pl/plparm.f90 no longer re-exports pl_view, so they carry the identical hard
compile error the commit claimed to have swept. Now routed via `USE plview`
like fp/fpmenu.f90 already was.
Also fixed:
- [A-MED] txnew/txexec.f90: the widened fallback `ierr_la /= 0` also swallowed
IER=-1 (BL/BX too small, :1092) and would have called LAPACK_DGBSV with the
very dimensions just found unsatisfied. Narrowed to `== -2 .or. > 0`, so the
dimension error keeps its loud path.
- [B-MED] eq_save's artefact check could not tell "written now" from "already
there": channel B reproduced a false success by saving once, setting MODEFW=3
(so FWOPEN refuses to reopen), and saving again -- the first run's file made
both the Fortran INQUIRE and the Python os.path check pass. EQSAVE now takes a
real IERR out-argument and propagates FWOPEN's failure; eq_api_save honours it
before falling back to the artefact check. Both call sites updated
(eq/eqmenu.f90:98, eq/eq_api.f90). New regression test reproduces the exact
repeat-save scenario and asserts the earlier file is left untouched.
- [A-MED] Stale docstrings in eq_mcp/tr_mcp still said the fd shim runs "at
startup (BEFORE any mcp/logging import)" and "at module load time", and that
the context manager is "effectively a no-op" -- false for an importer, which
is now exactly the case it protects. Rewritten.
- [A-MED] tr/trcomm.f90's idempotency-guard comment cited "tr_menu calls this
unconditionally at exit" -- the call 98ba1e2 removes. Now cites trmain and
tr_api_finalize.
- [B-LOW] "wm owns a local PT" in wmf/wmfn/wmseki was wrong: those trees have no
declared local PT, they rely on implicit REAL*8 typing. Comment corrected.
(Accurate as written for trm and wmx, which do declare one.)
- [A-MED] test_finalize_is_idempotent's docstring claimed to exercise the
double-DEALLOCATE guard; _ServerState.close() nulls the handle so the second
call never re-enters Fortran. Docstring now says what the test actually covers.
Disclosed but deliberately NOT changed here (for the PR thread):
- Restoring `DO NS=1,NSMAX` (finding 24) leaves NS_T/NS_He4 at 0 for NSMAX<4,
which several unguarded trx/ sites would index as PZ(0)/PA(0)/PIN(NR,0).
Upstream trm/ has always used this bound with the same half-guards, several of
those routines hardcode species 3/4 regardless, and both committed inputs pin
NSMAX=4 -- so NSMAX<4 was never supported. Reverting the bound would instead
re-break libnf's presence STOP, which is the defect being fixed.
- Channel B also found pre-existing defects this batch does not claim: NS_He5
never reset by tr_prep_ns; libnf CASE(1,12,13) vs the documented model_pnf=14;
eq/eqfile.f90's transposed PSI/DELPSI/HJT write-vs-read; tr/trloop.f90's two
bare `return`s; missing IOSTAT on equ_convert's READs; txmain never inspecting
IERR. All are worth issues; none are regressions from this PR.
Verification: full python suite 602 passed / 169 skipped / 0 failed. TOT still
TR_PRESENT=1. tr_m0904 dump still byte-identical to the pre-refactor baseline
(sha256 56d09887...cb9ad869). CI job re-verified 9/9 on a pristine clone.
Review note: codex was unavailable in this environment (the CLI rejects the
default model as too new, and the account cannot select gpt-5.2/gpt-5.2-codex),
so the second channel was an independent adversarial reviewer with a different
mandate rather than the usual codex pass.
|
Pushed 9 commits (head Each fix was reproduced before being written and re-verified after; where I could not reproduce a reported mechanism I say so below rather than patching on faith. What landed
Two things I got wrong, and correctedWorth stating plainly since both were in work I had already called verified. The new CI job could not have passed. My "all nine targets green locally" was a dirty-worktree false green — this checkout had #228-21 was only 2 of 6 sites. I migrated And one hole in my own The CI gap is the part I'd most like your view on
Proposed de-scoping, with evidenceBoth of these surfaced while fixing the PT/
Together with the items already moved to #229 and #224, and #227 item 4 (refuted earlier — Pre-existing defects worth their own issuesFound while reviewing this work; all verified NOT to be regressions from this PR, so I have not touched them:
Two disclosures on my own changes#228-24. Restoring Test coverage. Still blocked on the same thingThe workflows have still never run. The new head has its own run — Once CI is green I will regenerate |
Fixes the only pytest failure in the first CI run this PR was allowed to
execute (run 30196568304): `test_get_psi_rz_via_mcp`, on both 3.11 and 3.13.
743 passed, 1 failed.
Root cause is a dependency drift, not a code regression. The workflow ran a
bare `pip install mcp`, and the declared bound is `mcp>=0.9` with no ceiling,
so CI picked up the **mcp 2.0.0** release. Verified in a throwaway venv:
mcp 2.0.0 from mcp.server.fastmcp import FastMCP -> ModuleNotFoundError
(replaced by mcp.server.MCPServer / Server, different API)
from mcp import ClientSession -> OK
from mcp.client.stdio import stdio_client -> OK
The client half survives; the server half does not. That asymmetry is exactly
how the failure looked: this test imports only the CLIENT API, so the module
imported fine and the test ran, but the `python -m eq_mcp.server` subprocess it
spawns needs the SERVER half, hit `MCP_AVAILABLE = False`, printed "Python MCP
SDK (`mcp`) is not installed" and exited 2 — the client then reported the
uninformative `MCPError: Connection closed`. The six sibling MCP test modules
all check `MCP_AVAILABLE` and skipped cleanly; only this one had no guard.
Two changes:
1. Pin `mcp>=0.9,<2` in the workflow and in all seven server pyproject files.
Verified `mcp>=0.9,<2` resolves to 1.29.0 and that FastMCP/ToolError import
there, and ran eq_mcp + tr_mcp against 1.29.0: 83 passed, MCP_AVAILABLE=True
for both. (Local development is on 1.27.0, so the pin was checked against
the version CI will now actually install, not just mine.) Porting the seven
servers to the 2.x API is a separate piece of work, not this PR's.
2. Give the test the prerequisite guards its siblings have: client SDK
(importorskip), server SDK (`_srv.MCP_AVAILABLE`), and a built
`eq/libeqapi.so` (honouring EQLIB_PATH).
On the skip guard vs. CLAUDE.md's "never skip to cover up a broken test": the
pin is the fix — with a correct install the test RUNS, and it does. Confirmed
both directions:
mcp 1.27 + libeqapi.so built -> PASSED (runs, does not skip)
mcp 2.0.0 venv -> SKIPPED with the reason naming
mcp.server.fastmcp and the pin to apply
So the guard adds no blind spot in a correctly-installed environment; it only
converts an opaque "Connection closed" into a diagnosable message when a
prerequisite is genuinely absent. It also covers the missing-.so case, which is
what made this same test fail for me locally before I built libeqapi.so.
Full suite: 602 passed, 169 skipped, 0 failed.
Not addressed here: the mono-build job's `test_tot_ht6m_short` failure. That is
a genuine numerical divergence between libtotapi_mono.so and the default
libtotapi.so (the same baseline PASSES in the pytest job against the default
library), and it needs its own investigation.
…harden the guards Follow-up to c528b58, from the pre-push review of it. Three real gaps. 1. The remediation text pointed at the broken version. Every server printed error: Python MCP SDK (`mcp`) is not installed. pip install 'mcp>=0.9' which is the exact stderr line CI captured — so a user who hit the failure and followed the instruction would install mcp 2.0.0 and reproduce it verbatim. 44 unbounded `mcp>=0.9` strings across 25 files (14 in the seven server.py files, the rest in READMEs, docs/sphinx module pages and a presentation builder) are now `mcp>=0.9,<2`. The new test's skip reason already had the right string; it simply had not been propagated. 2. The subprocess had a fourth, unguarded prerequisite: PYTHONPATH. The test's sys.path inserts fix up the PARENT, so `_srv.MCP_AVAILABLE` was being read from a process that is not the one under test; the CHILD could still die with `ModuleNotFoundError: No module named 'eq_mcp'` and produce the same opaque "Connection closed" this file exists to prevent. The spawned server now gets PYTHONPATH explicitly, mirroring those inserts. Verified: the test passes with `env -u PYTHONPATH`, i.e. it no longer depends on the job-level variable. 3. `_resolved_so()` re-derived a narrower resolution order than the library it guards. `eqlib._ffi._default_lib_path` tries MONO_LIB_PATH (priority 0, above EQLIB_PATH), then EQLIB_PATH, then eq/libeqapi.so, then lib/libeqapi.so; the guard checked only the middle two, so a layout with the library under lib/ — or a MONO_LIB_PATH run — would have skipped a test that was perfectly runnable. It now calls `_default_lib_path()` directly, so the guard cannot drift from the loader again. Plus the tripwire the review asked for, which is what actually satisfies CLAUDE.md's "never skip to cover up" rule rather than merely arguing about it: `MCP_AVAILABLE` is set by a bare `except Exception`, so ANY future breakage of the FastMCP import path would silently turn all seven MCP suites green-by-skip. The workflow now asserts `MCP_AVAILABLE` right after installing the pin, so in the one environment where those tests MUST run, a skip becomes a hard failure. Checked both ways: the assertion fails in an mcp-2.0.0 venv and passes in the pinned one. Verification: test still PASSED (not skipped) in a correct environment, PASSED with PYTHONPATH unset, and the full suite is 602 passed / 169 skipped / 1 xpassed / 0 failed (772 collected — the earlier commit's "602 + 169" omitted the xpass, it was not a miscount).
Strict re-review at head
|
| Item | Status |
|---|---|
#228-2 MODELG=25 rejected by plcheck while eqfile.f90 dispatches it |
NOT FIXED (pl/plparm.f90 still MODELG>11 reject) |
#228-3 tr_eq_init (and tr_rip_eq) never reset by finalize/init |
NOT FIXED — now that #227-2 makes finalize→init a real reset, this is the remaining stale state on the MODELG=9 re-init path |
#228-4 MODELG=9 q-scaling error handling (ierr ignored, tr_rip_eq=0 divide, silent 30-iter exhaustion) |
NOT FIXED |
| #228-16 eq_iter01/eq_tst2 baseline drift | NOT FIXED (author: blocked on CI approval — now unblocked) |
4. New findings from this review (not previously reported in #227/#228/#229)
Numerics / physics
trx/in/test01.insilently disables fusion. Upstream replacedmodel_pnf=1withnnfmax=1, but the merged code derivesnnfmaxfrommodel_pnf(default 0) inset_usigmav_nf→trprepruns with fusion off. The committed referencetest01.pdfwas regenerated in that state. This is a merge-reconciliation inconsistency between upstream's nnfmax-driven convention and the fork's model_pnf-driven code. (The author flagged the coverage angle; the stronger point is that the shipped deck no longer does what its author intended.)txnew/txlib.f90FEM integral tables changed without any test coverage.fem_int_pointcase(7) sign pattern swapped, case(25) formula replaced (a'*b'*u*w→a*b'*u'*w), vector cases 7/14 added; case(25) is live in Ampere's law (txcala.f90:496). txnew has zero pytest/equivalence coverage and the new CI job is compile-only, so this physics change lands unvalidated. Deserves at least a recorded baseline decision.trx/libnf.f90:475— newly publicsigmav_nf_intis unusable:pm_localis declaredINTEGER(mass ~6.6e-27 truncates to 0 → integer zero divide on first call) and it uses the product species mass where the reactant reduced mass is needed. Pre-existing code, but this PR promotes it to the PUBLIC API (alongsidePUBLIC sigmav_nfb_int, which names a procedure that doesn't exist — it compiles as an implicit REAL module variable; = PR #226 follow-up: fix non-MCP regressions in PL/TR/TRX/WM/W1 #228-26).pl/plparm.f90/trx/trparm.f90—pt_save(nsmax)/pu_save(nsmax)automatic arrays are sized with the pre-read NSMAX but indexed with the post-read NSMAX on the error/retry paths: a namelist that raises NSMAX can cause stack OOB reads (error path) and OOB writes (interactive retry path). This makes PR #226 follow-up: fix non-MCP regressions in PL/TR/TRX/WM/W1 #228-25 concrete and user-input-triggerable.
Lifecycle / contracts
eq/eq_api.h:21-22,164-166and the eq_mcpsavetool docstring still say "Returns EQ_OK unconditionally; caller MUST verify file existence" — the exact opposite of the post-10fb296fimplementation. The header is the C contract and the docstring ships to MCP clients; both will cause callers to write dead verification code and miss real error codes. Five-minute fix, disproportionate value.- MCP servers still mutate host state at import:
_libc.setvbuf(_c_stdout, None, _IONBF, 0)runs at module scope, permanently switching the importing process's C stdout to unbuffered.test_import_isolation.pypins only fd 1 +sys.stdout, so this residual is untested. Also, the0bf7e399rework newly routes in-process importers through the_gfortran_flush(unit6)call that the module's own comment (tr_mcp/server.py:495-498) describes as "SIGABRT ~10-40% of the time" — the code and the comment can't both be right; either drop the flush on the non-server path or correct the stale comment. eq/eq-eqdsk.f90:58— unguarded debugWRITE(6,...)dumps the full PPPS array on every geqdsk load, including the library path (eq_runwith MODELG=5/25), spamming the Python host's stdout. The sibling print added ineqcalq.f90is correctly NPRINT-guarded; this one is not.
Build / hygiene
equ/Makefile:make -C equ libequ.acannot build from a clean tree (equcomm.o/aaa_modordering); CI works around it with a pre-build step — the fix belongs in the Makefile as a real prerequisite. Alsoequ/equintf.f90andequ/equplintf.f90are still fixed-form inside (renamed only) — dead files, but the Makefile carries dep lines for them; convert or delete.wf2dt/Makefile:14-15: build switched fromOFLAGStoDFLAGS -Wall(debug flags now compilelibwf.ain CI) — looks like an upstream working-state toggle committed by accident.equ/equ_convert.f90:32: new code with a library-reachableSTOP(issue Fortran STOP in library-reachable code paths — enumeration + migration #142 discipline) + seven IOSTAT-less READs;equ/equsub.f90ships ten@@@pointNaN-probe debug lines;fpx/fpcoef.f90carries six unconditional@@@ pointdebug WRITEs (= PR #226 follow-up: fix non-MCP regressions in PL/TR/TRX/WM/W1 #228-11, still present).trm/: plcomm's PROF* scalars becameDIMENSION(NSM)arrays, silently changing trm's unformatted save-file record layout with no version marker (trm is dormant, but round-trip compat is broken if built).- Repo hygiene:
trx/pdf/test01.pdfgrew to 171 KB (>100 KB binary),doc/task_matrix_solver.pdfis a committed regenerable binary.
5. Codex second opinion
Codex reported one HIGH (wm/wmcomm.inc PT un-renamed → wmbooz.f breaks → make -C wm libwm.a fails). Refuted against the actual PR head: wmbooz.f:570 is already PTAV at pr-226; a word-boundary sweep finds zero live bare-PT uses in wm/ (remaining hits are string literals and wmtrfile.f, which includes ../tr/trcomm.inc, not wmcomm.inc); and empirically the "build modules not covered by pytest" CI job — which builds wm/libwm.a — already passes on this PR. The derived MED falls with it.
One salvageable point (LOW): wm/wmcomm.inc itself still does USE plcomm / USE dpcomm without the pt_pl=>PT rename that wmf/wmfn/wmseki received. Harmless today (no bare PT uses remain), but under implicit typing any future bare PT in fixed-form wm code silently binds the shared array again. Recommend adding the same rename for symmetry.
Codex independently confirmed clean: all 10 conflict files, the PT rename across ti/trm/wmx/wmf/wmfn/wmseki, the eq_save/get_psi_rz ctypes ABI, MCP lifecycle, regen-baselines.yml, no tolerance relaxation, no skip additions, fixture sizes.
6. Verified clean (so nobody re-litigates these)
- Merge topology claim correct: merge-base = current
developtip, 0 behind. - equ/ fixed→free-form migration: all 16 renamed pairs token-normalized and compared — exactly token-identical (IMPLICIT, DATA, formats preserved). The single genuine col-72 resurrection (
equsub.f90:1327printingistepinstead ofi) restores the obvious original intent, diagnostic-only. - plcomm PT containment and the pl_view move: exhaustive sweeps found both complete (17/17 call sites linked; the only unresolved caller,
trn/, was equally broken atdevelop). - eq_save error path end-to-end (incl. the MODEFW interactive-prompt hazard being closed by
eqinit'sMODEFW=0); TRCOMM ownership (no leak, no double-free, idempotent via theALLOCATED(PNSS)sentinel); EQLOOP under-relaxation algebra; NTVMAX/NMAX 200→400 consumers (compile-time max NTVM=1025, all clamped); 8→15 species-param growth consumers;trbpsdtr/trbpsd.f90 + bpsd/bpsd_plasmaf.f90 OOB cascade with eqdata.TST-2 (follow-up to #194) #203-B allocation fix; the trexec NVTM time-centering fix;wrcalpwr3-way blend (OOB clamp intact, no mixed old/new dx, power conserved in both integrals); LAPACK shim parity + block-tri fallback (loud, never silent-wrong); SAVE-workspace regrow logic in txcalv/txcala; REGEN capture cannot mask failures; regen-baselines.yml security posture;.gitignoreunion.
Suggested merge gate
- CI green: pin
mcp<2(or port to mcp 2.0) + regenerate the committed eqdata fixtures / add eqdata generation to the mono job (§1). - wrx: physics sign-off record for the 2.1e-2 deposition shifts; comparator arrays gap fixed or tracked; arrays2 layout canonicalized (§2).
- The four open PR #226 follow-up: fix non-MCP regressions in PL/TR/TRX/WM/W1 #228 P1s: 2, 3, 4, 16 (§3).
- Contract docs:
eq_api.h+ MCP save docstring corrected (§4).
Everything else above is tracked-issue material rather than a blocker, including the new MED/LOW findings and the wm/wmcomm.inc symmetry hardening.
Review conducted with Claude Code (5 parallel subsystem agents + fix-verification agent + Codex CLI 0.146.0 second opinion). CI evidence from run 30196568304.
The mono job's `test_tot_ht6m_short` fails with 210 mismatches while
the identical test passes in the pytest job, on the same runner image,
the same compiler and the same committed baseline. The cause is a
stale input, not the monolithic library.
`_isolated_cwd_with_eqdata` (python/totlib/tests/test_equivalence.py
:103-115) stages eqdata from two tiers:
tier 1 test_run/test_output/<case>/eqdata-* copied always
tier 2 python/totlib/tests/fixtures/eqdata-* copied only if absent
The pytest job runs "Generate Layer 1 ... eqdata baselines", so it
consumes a freshly produced tier 1. The mono job never ran that step
and `test_run/test_output/` is untracked, so it fell through to the
committed tier-2 fixture.
Decoding both files as gfortran unformatted-sequential gives 28 records
of identical length, differing first in the twelfth record (1-based;
records 7-11 are the five 21-double PSIPS/PPPS/TTPS/TEPS/OMPS arrays),
which eq/eqfile.f90:56 writes as
NSGMAX,NTGMAX,NUGMAX,NRMAX,NTHMAX,NSUMAX,NRVMAX,NTVMAX
tier 1 (32, 32, 32, 50, 64, 65, 50, 400)
tier 2 (32, 32, 32, 50, 64, 65, 50, 200)
The differing field is NTVMAX, as an int32. EQRTSK reads it back out of
the blob, so the committed fixture pins the equilibrium to the
NTVMAX=200 era regardless of what eqinit.f90 now defaults to (400, at
eq/eqinit.f90:269). The fixture was committed in 615959e (2026-05-14);
the baseline was regenerated against the CI compiler in d1f7f9e
(2026-06-29), 46 days later. tot_ht6m_short is the only case that reads
eqdata -- pl/plinit.f90:384 defaults MODELG=2 and only
test_run/inputs/tot_ht6m_short.trparm:2 sets modelg=3 -- and
tot_demo2014_short, same job and same .so, passes.
The library is not numerically wrong. compare_metrics.py:205 truncates
at `errors[:50]` and pytest echoes the failure block twice, so the
log's 100 `rel_err=` lines are 50 DISTINCT fields printed twice (the
two "... and 160 more" markers confirm 50 of 210). Comparing those 50
against the PRE-regeneration baseline (d1f7f9e^):
50/50 agree to within 1e-10, worst relative difference 1.076e-13
0/50 agree with the current baseline
and the old->new baseline diff is exactly 210 fields -- the reported
mismatch count. The sample is biased, not random: 10 scalars plus
profile[0..9], the innermost 10 of 50 radial points, so 160 of 210
fields were never examined and cannot be from this log. But "the mono
image reproduced the older equilibrium" predicts precisely a 210-field
disagreement with the new baseline, which is what is reported, at
~1e-13 -- three orders inside the 1e-10 gate -- because that is the
equilibrium it was handed.
So the mono job now runs the same generation step, and both jobs print
the tier-1 and tier-2 digests so this class of drift is visible in the
log rather than inferred from a failure.
Regenerating the fixture would also fix today's symptom, and it should
still happen (it is the same NTVMAX-read-back mechanism already
reported upstream as part of the EQ baseline work). It is not a
substitute: a committed blob cannot track eqinit.f90, so it will go
stale again on the next such change, whereas generating the input in
the job cannot. The fixture stays as the local-run fallback and as what
test_mono_bpsd_smoke.py loads directly.
`make -C tot libs` precedes `make -C tot tot`, as in the pytest job.
This job builds only PIC artefacts, and tot/Makefile's `tot` target
pulls in the non-PIC chain: $(LIB_MTX) -> ../mtxp/libmtx*.o, whose
first compile is `USE task_kinds` from lib/libtask.a, and $(LIBS) lists
libgrf.a before libtask.a although libgrf needs task_kinds.mod. `libs`
encodes the correct order in one recipe.
Also un-gate the four mono-specific steps (default-.so build, Layer-C
BPSD smoke, routing sanity, pipeline coupling). They answer different
questions from the equivalence test -- does mono share one BPSD broker,
route all six wrappers, couple through the pipeline -- and are most
diagnostic exactly when the equivalence test is red. On the current
head all four were skipped, so that push carried zero evidence about
any mono-specific claim.
The condition is `!cancelled() && steps.mono_equivalence.conclusion !=
'skipped'`, not a bare `!cancelled()`. A bare one also means "run after
a failed BUILD step", and on that path `Set up Python` and the pip
install (still success()-gated) have skipped, so the four steps would
reach `python -m pytest` with no interpreter and report three red steps
blaming Python tooling for a Fortran failure. `conclusion != 'skipped'`
is true exactly when the equivalence step ran, so a build failure stops
the job where it always did while an equivalence FAILURE still yields
the evidence. The three test steps additionally assert their .so inputs
exist, so a missing library fails loudly instead of degrading into a
green "3 skipped" via skipUnless. The 1e-10 gate is untouched: the
equivalence step's failure still fails the job.
ht6m's eqparm/trparm are now copied hard instead of `|| true`, in both
jobs. Losing .trparm silently would run from defaults, produce a
plausible non-empty eqdata, pass `test -s`, and resurface as
library-looking drift -- the exact misattribution this commit removes.
demo2014 ships neither companion and stays best-effort.
timeout-minutes 15 -> 20 for the added non-PIC chain (measured below).
Verified from a PRISTINE clone, in the mono job's own tree state
(macOS, gfortran-15) -- after its PIC chain, six per-module .so and
libtotapi_mono.so, with zero non-PIC artefacts present:
- `make -C tot tot` alone FAILS: "Fatal Error: Cannot open module
file 'task_kinds.mod'". So without the `libs` line this change
would have turned an informative red into a build error.
- with `make -C tot libs` first, the step exits 0 in 56s, and
libtotapi_mono.so plus all five per-module .so are byte-identical
before and after.
- both digest lines print as intended.
- python/totlib/tests with the CI flags: 165 passed, 2 skipped; the
full totlib + mcp-servers surface: 464 passed, 9 skipped.
An earlier revision of this message cited a sha256 check of
libtotapi_mono.so as evidence of non-perturbation. That check ran on an
incrementally-built tree where `tot/tot` already existed, so the make
invocation was a no-op and the evidence was vacuous -- it is also why
the missing `libs` line did not surface locally. The pristine-clone
runs above replace it.
The equivalence tests are Linux-canonical (k-yoshimi#213) and skip on macOS, so
the fix itself can only be confirmed by CI. The two digest lines make
that confirmation one glance rather than another round-trip.
Review: the in-house reviewer found the missing `make -C tot libs`, the
four-vs-two step inconsistency, the vacuous sha256 evidence and a
record ordinal off by one. A second, independent multi-lens review then
found the Python-provisioning hole in the un-gating and the 100-vs-50
double count above. All are fixed here. The codex second opinion this
repo's CLAUDE.md normally requires was unavailable -- the CLI rejects
both its default model and an explicit override with "not supported
when using Codex with a ChatGPT account" -- so that review stood in for
it; disclosed rather than silently skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes k-yoshimi#190. Addresses the "EQ baselines need regeneration" note that 0254aa9 left for itself. All six committed eqdata fixture paths were pinned to the NTVMAX=200 era. They are FOUR distinct blobs -- eqlib/ and trlib/ carry byte-identical copies of eqdata.ITER01 and eqdata.TST-2 -- and each decoded to a 32-byte NSGMAX..NTVMAX record (eq/eqfile.f90:56) ending in 200, while eq/eqinit.f90:269 defaults to 400. EQRTSK (eq/eqfile.f90:162) reads NTVMAX back OUT of the blob and overwrites the in-memory value, so a stale blob silently reverted the parameter for every consumer. That is what failed the mono job's tot_ht6m_short with 210 mismatches until a12af23 made the job generate its own input. 0254aa9 (2026-06-25) said so itself: "graft NTVMAX 200->400 (14f0a09) ... Numerics shift for stiff/coupled cases by design -> re-baseline EQ. NOTE: EQ baselines need regeneration (eqinit NLPMAX/NTVMAX + eqcalc...)". The drift is three grafted changes, not one: NTVMAX 200->400, NLPMAX 20->100 (eqinit.f90:280) and eqcalq NMAX 200->400. Fixture and baseline are inseparable. eq_iter01_params.py / eq_tst2_params.py / tr_iter01_params.py / tr_tst2_params.py are all MODELG=3 with KNAMEQ="eqdata.*"; there is no tier-1 generation step for them in python-tests.yml, so those four 1e-10 gates run off the committed blobs. Replacing the blobs alone would turn four green gates red. Both come from ONE run on ONE toolchain here. ## Generated on the canonical platform, because it has to be Neither local option qualifies: this Mac is gfortran 14.2/15.2, the lab Linux host is gfortran 11.4 / Ubuntu 22.04, and docs/baseline-policy.md names the canonical platform as the Ubuntu CI runner with gfortran 13.x. The size of that gap is not academic -- a macOS-generated eqdata.TST-2 run against the committed Linux baseline gives 10 mismatches at 4.011e-10, 4x over the 1e-10 tolerance, and it is invisible from the Mac that produced it because the gate skips on non-Linux (k-yoshimi#213). So both halves come from regen-baselines.yml run 30514856222 on ubuntu-24.04 / gfortran 13.3.0. Cross-check that this is the same output python-tests.yml produces: the regenerated eqdata-HT6M hashes 47708f9bf255e24b105542bd7b93429a9e5ae32a779282ee3debcb728447c052, which is byte-identical to what the pytest job's own tier-1 generation step printed in run 30441680544. Two independent workflows, same bytes. ## What changed, measured Six fixture paths, four blobs, each now header=(32,32,32,50,64,65,50,400), 28 records, 45956 bytes; the eqlib/trlib pairs still byte-identical (cmp). Five baselines change. Two do not, and that is worth stating because both could be mistaken for stale: tot_demo2014_short's predates 0254aa9 (14659b3, 2026-05-12), while tot_ht6m_short's was already regenerated against the CI compiler four days AFTER it (d1f7f9e, 2026-06-29): eq_iter01 204 fields worst 6.371e-04 @Profile[45].QPS eq_tst2 348 fields worst 3.157e-01 @scalars.ZAXIS eq_jt60 345 fields worst 9.210e+00 @scalars.ZAXIS tr_iter01 311 fields worst 2.360e-02 @Profile[47].AJ tr_tst2 211 fields + scalars.AJRFT (new key) tot_ht6m_short 0 fields (already regenerated at d1f7f9e) tot_demo2014_short 0 fields (MODELG=2 -- inert to NTVMAX) eq_iter01's 204 matches the count reported from clavius exactly, which is independent corroboration that this is the same drift. The two large RELATIVE diffs are numerical noise, not physics: ZAXIS is the magnetic-axis vertical position, zero for an up-down-symmetric equilibrium. eq_tst2 moves 1.298e-11 -> 8.883e-12 and eq_jt60 -1.440e-13 -> 1.182e-12 (it even changes sign). A ~1e-13 denominator makes the ratio meaningless -- and neither is even gate-relevant: compare_metrics.py:70-77 switches to ABSOLUTE error when both values sit under _NEAR_ZERO_THRESHOLD = 1e-10, so these compare at 4.10e-12 and 1.33e-12 against the same 1e-10 bound, passing with four decades of margin. The table lists the ratios only because that is the convention for the other rows (denominator = the OLD baseline value). Every physically meaningful EQ scalar moves by 4.7e-5..3.0e-4 -- RAAVE, PVOL, QSURF, PSITA, BETAT, BETAP, QAXIS in ascending order, five of the seven at or above 1e-4. On the TR side the largest scalar drift anywhere in this commit is TAUE2: 4.29e-3 (tr_tst2) and 3.70e-3 (tr_iter01). All of it is what doubling the flux-surface integration resolution should do, and what 0254aa9 predicted. The drift signature confirms it is quadrature and not physics. In eq_iter01 the Grad-Shafranov solve is BIT-UNCHANGED -- PPS, TTS, PSIP, RAXIS, ZAXIS, PSI0, PSIPA, RIPX all identical -- and only flux-surface-integral quantities move. In tr_iter01 and tr_tst2, RN (density) changes in 0 of 50 rows while AJT (total current) moves 5.0e-5 and local AJ up to 2.36e-2: the current redistributes and the total is conserved. Every profile varies smoothly in radius with a single sign-change node, and profile[47].AJ -- the largest non-zero drift in the commit -- sits immediately beside one (AJ[46] is negative, AJ[45] and AJ[47] positive), so it is a small-denominator artefact rather than a localized anomaly. There is no spike anywhere. ## The k-yoshimi#190 xfail is removed, and that is not optional python/trlib/tests/test_equivalence.py's tr_tst2 marker read "baseline ... is missing AJRFT ... baseline regen is blocked by upstream eq_tst2 drift (~3e-9 > 1e-10)". Both halves are resolved here: the regenerated baseline carries 14 scalars including scalars.AJRFT (verified: it is the one key the regen ADDS), and the eq_tst2 drift WAS the stale blob -- measurably so: the old->new eq_tst2 PSIP drift peaks at 3.0157e-09 at NR=1, matching the "~3e-9" the removed marker and regen-baselines.yml:3-5 both quote. (k-yoshimi#197 attributed that figure to clavius-vs-CI compiler drift instead; both explanations are cured by regenerating on the runner, so the direction holds either way.) One thing local work could NOT establish: the baseline comes from the standalone Fortran binary while the gate runs the libtrapi.so wrapper. That binary->wrapper delta is empirically under 1e-10 for eq_iter01, eq_tst2 and tr_iter01 -- their gates are green today against binary-produced baselines -- but for tr_tst2 it had never been measured, because the test has been xfail since k-yoshimi#190. Removing a strict xfail on a test never observed to pass is the one change here that could take a green PR red. So it was measured, on Linux, before this was pushed: probe run 30516419971 (HengyuLi-Ozaki-lab/task @ probe/eqdata-gates, this exact tree plus a throwaway `probe/**` trigger, since python-tests.yml only fires on develop / master / chore/pre-push-hook-worktree-compat). All four jobs green: python/eqlib/tests/test_equivalence.py .. 2 passed python/trlib/tests/test_equivalence.py .x. 2 passed + 1 xfail python/totlib/tests/test_equivalence.py .. 2 passed Not one `s` among them, so all four blob-reading gates actually RAN rather than skipping. The single remaining trlib xfail is test_m0904 (k-yoshimi#224), confirmed from the XFAIL list; test_tst2 therefore passes. Suite totals 752 passed / 16 skipped / 3 xfailed / 1 xpassed, against 751 / 16 on the current PR head af19740 -- the +1 is test_tst2, and the three xfails are exactly k-yoshimi#222 (fp_jt60), k-yoshimi#223 (ti_w) and k-yoshimi#224 (tr_m0904). The 1 xpassed is k-yoshimi#189, strict=False, and it xpasses identically with the OLD blob. With strict=True and no xfail_strict override anywhere in the repo, leaving the marker would make a now-passing test_tst2 report XPASS(strict) and fail CI red -- trading one red for another. The k-yoshimi#224 tr_m0904 marker stays: it is MODELG=2 with no KNAMEQ, so it is not downstream of eqdata at all. ## Two pre-existing run_tests.sh defects that blocked this entirely Both are in the shipped fix because both make run_tests.sh unusable on any graphics-free build, not just in the regen workflow. Neither had ever been triggered: python-tests.yml invokes the binaries directly and never calls run_tests.sh, and regen-baselines.yml had never run anywhere. 1. Success was gated on `grep -q "CLOSED"`, the only path that set COMPLETED_TESTS. "CLOSED" comes from GSAF's GSCLOS, which is not in this repo -- the sole in-tree occurrence is wim/wimmain.f90:35, unrelated. With an empty GFLIBS (every CI build) the Makefiles link <mod>_static_stubs.o, whose GSCLOS is an empty subroutine (eq/eq_static_stubs.f90:227-228, likewise tr/ and tot/). So every case reported "FAIL (no CLOSED message)" and every dependent case was skipped. Now a clean exit that produced the requested <module>_regress.dat counts too; "CLOSED" still works unchanged. 2. The REGRESSION branch did not set COMPLETED_TESTS, so a drifted case poisoned its dependents. Same conceptual error: a VERDICT used as an ARTEFACT-AVAILABILITY signal. COMPLETED_TESTS answers "did this produce output a dependent can consume?"; a metrics drift says the BASELINE is out of date. That distinction matters most for a regeneration run, where every case is expected to drift. FAILED is still incremented, so a drift never reads as a pass. Found by running it: fix 1 alone got run 30514627136 as far as "eq_iter01 REGRESSION ... ERROR: dump file test_run/test_output/tr_iter01/ tr_regress.dat is empty or missing". Both decision branches were then verified directly (run_tests.sh cannot execute on this host -- it needs bash 4+ for `declare -A` and GNU `timeout`): no CLOSED, exit 0, dump present -> PASS, COMPLETED=1, FAILED=0 dump present, metrics drift -> REGRESSION, COMPLETED=1, FAILED=1 dump present, metrics match -> PASS, COMPLETED=1, FAILED=0 no dump, exit 0 -> FAIL, COMPLETED=0, FAILED=1 no CLOSED, exit 2, dump present -> FAIL (exit code: 2), COMPLETED=0 exit 124 -> TIMEOUT (unchanged) ## regen-baselines.yml Gains the eqdata collection step (asserting NTVMAX == 400 per blob, and requiring exactly ONE 32-byte record so a small-grid regen cannot have REAL(8) mantissa bytes read as the header), the FIXTURES default repeated in env: (inputs.fixtures is empty on non-dispatch events), a guard against a whitespace-only value, blob-producing cases ordered first, and `if: always()` on collect and upload so one bad fixture does not discard the rest. It remains UNREACHABLE and that is left as a maintainer decision: workflow_dispatch resolves only on the default branch, and master carries no .github/ at all, so this workflow has never run here. The artefacts above were produced by adding a temporary `push: branches: ['regen/**']` trigger on a scratch branch of my fork (HengyuLi-Ozaki-lab/task @ regen/eqdata-2026-07, run 30514856222). That trigger is deliberately NOT included -- shipping a push trigger nobody asked for is your call. Landing .github/ on master makes dispatch work permanently. Verified locally (macOS; all four blob-reading 1e-10 gates -- eq_iter01, eq_tst2, tr_iter01, tr_tst2 -- skip here per k-yoshimi#213, so they can only be confirmed by CI): - all six paths: NTVMAX=400, exactly one 32-byte record, 28 records, 45956 bytes; eqlib/trlib pairs byte-identical - tr_tst2 baseline: 14 scalars, AJRFT present - with test_run/test_output REMOVED first, so tier-1 cannot shadow tier-2: eqlib + trlib + totlib + eq_mcp + tr_mcp = 344 passed, 20 skipped, 1 xpassed - that 1 xpassed is k-yoshimi#189 test_NSMAX_in_range, strict=False, and it xpasses identically with the OLD blob -- pre-existing and environment-dependent, not introduced here (checked by stashing the fixtures and re-running) - the fixtures directories stay clean afterwards (eq_mcp chdir's into one) Not touched, worth their own issues: equ/equfile.f90:53 hardcodes NTVMAX=200 in a second in-tree writer of this same 28-record format, with NSGMAX/NTGMAX/NUGMAX=64 and NRMAX=100 that do not even match eq's 32/32/32/50; and docs/sphinx/modules/eq/{en,ja}/parameters.md still document NTVMAX as 200. Review: one in-house reviewer, whose findings on the preceding scratch-branch commit (the GSAF/CLOSED defect, a missing `if: always()`, the first-32-byte-record ambiguity, the artifact layout, and a miscount of stale baselines) are all folded in here. The codex second opinion CLAUDE.md normally requires is unavailable -- the CLI rejects both its default model and an explicit override with "not supported when using Codex with a ChatGPT account" -- disclosed rather than silently skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Delivers the assertion promised with the EQ baseline regeneration ("add the
assertion that stops that false-negative recurring"). Auditing the previous
commit against that promise showed it had not been added: the only NTVMAX
check lived in regen-baselines.yml's collect step, which runs when somebody
regenerates and so validates a regen's OUTPUT -- never the repository's
current state. Nothing was watching the committed files, which is the hole
that produced this episode.
## The guard
python/eqlib/tests/test_eqdata_fixture_freshness.py, over the six committed
paths (four distinct blobs; eqlib/ and trlib/ hold byte-identical copies):
- every field of the NSGMAX..NTVMAX header matches eq/eqinit.f90's current
default, PARSED from source rather than hardcoded, so it fires in both
directions -- stale fixture vs current source, and bumped source vs
current fixtures. Both measured.
- record framing intact: 28 records, header == trailer, exactly one 32-byte
record (a 4-element REAL(8) array is also 32 bytes).
- the eqlib/ and trlib/ copies stay byte-identical -- one artefact, two
paths, and refreshing only one would have those packages silently testing
different equilibria.
- none of the inputs that can carry an `&eq` namelist overrides any of the
eight. All eight are /EQ/ members (eqinit.f90:446,448), so an override is
legal -- and would make a CORRECTLY regenerated fixture fail the header
check, with advice ("regenerate") that could not fix it. Failing on the
input names the real cause.
Why a test and not a workflow check: eq/eqfile.f90's EQRTSK reads NTVMAX back
out of the blob and overwrites the in-memory value, so a committed fixture is
a parameter injector that wins over eqinit.f90. When NTVMAX went 200 -> 400
the fixtures pinned every consumer to the old value, and the eqlib/trlib
1e-10 gates could not detect the change they exist to detect: they fed the
solver the old parameter and compared against baselines captured with it. The
mismatch surfaced only where the fixture was NOT used -- the mono job's
tot_ht6m_short, 210 mismatches -- and read as a library bug for a while.
It also closes a coverage gap the regeneration left: both CI jobs generate
tier-1 eqdata and _isolated_cwd_with_eqdata copies tier 1 unconditionally, so
the committed eqdata-HT6M and eqdata.demo2014 are shadowed in every numerical
test, and test_mono_bpsd_smoke.py asserts only return codes. Those two blobs
had no gate at all.
The same source-parse rule is now in regen-baselines.yml's collect step,
including re.IGNORECASE and the fixed-form comment markers. A literal 400
there would deadlock the remediation this test directs people to: bump eqinit
to 800, the test fires and sends you to the workflow, the regen produces a
correct 800 blob, and the collect step refuses it as "expected 400".
## test_run/run_tests.sh
Two changes, both prerequisites rather than cleanup.
check_regression.sh returns 1=drift, 2=dump missing/malformed, 3=baseline
missing. All three were collapsed into "REGRESSION (metrics drift)", so a run
producing no metrics reported drift and -- per the COMPLETED_TESTS rule --
unblocked its dependents. Now: rc=1 is drift and stays COMPLETED so dependents
proceed (that rule cured a real cascade, measured on run 30514627136); rc=2 is
FAIL and not complete; rc=3 is FAIL but complete, because it is reachable only
after the dump was found and metrics.json written (check_regression.sh:50,
:61, :76) -- marking it incomplete would break the BOOTSTRAP case a
regeneration workflow exists for, where a new eq_* case has no baseline by
definition and its tr_* dependent would never run to produce one.
rc=3's COMPLETED is gated on eqdata.* actually existing. exit 3 proves the
DUMP exists, which is weaker: the dump is written at the tail of EQCALQ (the
menu's `r`), eqdata.* by EQSAVE (the menu's `s`), later. A bootstrap script
reaching `r` but not `s` would otherwise let its dependent run with no
equilibrium file -- and under regen-baselines.yml that is not even red, since
the run_tests.sh call is `|| true`'d and the dump and jq gates both pass, so a
metrics.json computed without an equilibrium would be uploaded as the artifact
intended to BECOME the baseline.
Second: the pre-run invalidation now clears eqdata.* / *.gs as well as the
dump, and runs immediately after `mkdir -p "$test_dir"` -- before the
dependency copy and the tot input staging. Placed after them, as first
written, it deleted the dependency's eqdata and would have broken
tr_iter01/tr_tst2 outright.
## docs/baseline-policy.md, and a correction that reaches beyond this commit
The provenance table gains an explicit basis column -- measured / stated /
unconfirmed / unknown -- after two earlier revisions of it asserted
attributions the tree does not support.
One correction matters beyond this repo's docs: **the CI compiler never
drifted.** d1f7f9e's "gf13.2", repeated since in commit messages, workflow
comments and an earlier version of this table, reads the apt metapackage
revision `4:13.2.0-7ubuntu1`. The run that commit cites, 28327720495
(2026-06-28), logs `GNU Fortran (Ubuntu 13.3.0-6ubuntu2~24.04.1) 13.3.0` --
identical to run 30516807586 today. Rows 1 and 2 are the same compiler.
Corrected in the table and in python-tests.yml; the PR k-yoshimi#138 narrative at
python-tests.yml:51-57 is left as history, with the correction at :58-61.
## Corrections to 19a552b's message
That commit is pushed and I am not rewriting a PR head under review, so these
belong here. Two of its statements contradict its own diff -- the direct cost
of amending twice without re-review:
- it describes "`if: always()` on collect and upload"; that commit changed
collect to `!cancelled() && steps.metrics.conclusion != 'skipped'`.
- under "Not touched" it lists the sphinx parameters.md files as still
documenting NTVMAX as 200; the same commit changes both to 400.
It also omits the dump invalidation and the tx FAIL-message split from its
run_tests.sh section, and never names the regen-baselines timeout raise
(20 -> 30) or the new provenance table at all.
## Verified
Seven review rounds ran on this work before it was proposed for push --
none of them happened for 19a552b, which is how that commit's message came
to contradict its own diff. They found, among others, the hardcoded 400
above, the invalidation ordering, the rc=3 collapse, the eqdata gating, a
parser that had IGNORECASE on one side only, and the compiler-drift error.
All are fixed here.
- python/ with the CI flags, test_run/test_output removed first so tier-1
cannot shadow tier-2: 725 passed / 66 skipped / 1 xpassed. The xpass is
k-yoshimi#189, strict=False, pre-existing and unaffected.
- the new file: 20 passed. Failure directions exercised by mutation --
pre-regeneration blob, bumped source, truncated file, deleted file, and an
input override (`nrvmax=100` in tot_ht6m_short.eqparm, which the guard
reports with its file:line).
- run_tests.sh's verdict logic branch by branch against synthetic inputs
(the script needs bash 4+ and GNU timeout, so it cannot run on this host):
rc=0 PASS/complete; rc=1 REGRESSION/complete; rc=2 FAIL/incomplete;
rc=3 with eqdata FAIL/complete, without eqdata FAIL/incomplete; no dump
FAIL; exit 124 TIMEOUT.
- both source parsers resolve all eight fields to exactly one value each.
A later round caught three more, all of them mine: a code comment
claiming `tot_ht6m_short.trparm` "produced a false positive" when it never
did (that file sets nsmax/ntmax/ntstep/ngrstp -- no header field, and the
pre-change test passes: 20 passed); the eqdata gate accepting files this
script had itself staged, and missing eq/eqinit.f90:100's bare `KNAMEQ =
'eqdata'` default; and a stale line reference plus an orphaned 15-line
comment block that still described an `rm` this commit had moved 80 lines
earlier -- the kind of leftover that invites someone to move it back.
The gate is now `find -maxdepth 1 -type f -name 'eqdata*' -newer .pre_run`,
which is the difference between "an eqdata file is present" and "THIS run
wrote one". The sentinel is written immediately before the binary, AFTER
every staging step: written before them, as it first was, it accomplished
nothing -- `cp` without `-p` stamps the destination with the current time,
so every staged file came out newer than the sentinel and satisfied the
check the sentinel exists to defeat.
Verified by replaying the script's own command sequence (rm, cp, sentinel,
optional write) rather than a hand-built directory: a staged dependency blob
with no binary output -> blocked; the same plus a file the run wrote ->
complete. With the sentinel in its original position the first case returns
complete, which is the defect.
Six times across six consecutive review rounds, the defect was me
asserting an observation I had not made:
- a compiler "drift" between the June and July CI runners, labelled
measured; the logs of both runs say gfortran 13.3.0
- an `ohtaka gf8.5` provenance asserted for wrx_jt60. That attribution is
real and in-tree -- 36586de's body says "Generated on LINUX x86_64
(ohtaka, gfortran 8.5)" -- but 36586de touched only wrx_demo and
wrx_iter01, so it never transferred to wrx_jt60
- a code comment saying `tot_ht6m_short.trparm` "produced a false
positive"; it never did, and the pre-change test passes
- a claim that trparm is applied after EQSAVE, when tot/totmain.f90:65
reads it before tot_menu at :75 -- inverted
- a "measured" gate result obtained from a hand-built directory the
script cannot produce
- and, in the comment rewritten to remove the third of these, a fresh
one: "setting a transport or kinetic radial grid there ... never
reaches the blob header". tr/trmetric.f90:33-38 pushes nrmax+1,
nthmax=64 and nsumax=0 INTO the /EQ/ namelist via eq_parm(2,...) when
modelg is 3, 5 or 8 -- and tot_ht6m_short.trparm:2 sets modelg=3. One
`grep -rni nthmax tr/` refutes it. What actually makes the exclusion
safe is ordering, which the round before had made me delete rather
than restate at the right level
All six were caught by review, not by me. The changes they justified were
each right on other grounds; only the justifications were invented. Two
further instances of the same kind -- an unverifiable clavius compiler
version, and a ~4e-10 figure I had been repeating as my own measurement --
I did catch myself, and both are now marked as documented-not-established
rather than removed.
Recorded here rather than quietly fixed, because it is the same failure
mode that put a stale fixture in this repo for six weeks: a value asserted
from prose instead of read from the thing it describes.
Also in this commit and not previously named: a one-space realignment of the
NLPMAX row in docs/sphinx/modules/eq/{en,ja}/parameters.md, cosmetic after
19a552b widened that value.
Still open, without issue numbers: equ/equfile.f90:53 hardcodes NTVMAX=200 in
a second in-tree writer of this format (its 64/64/64/100 geometry means the
dimension assertions would reject such a blob, so this is a documentation gap,
not an exposure).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributing the bpsi↔fork merge baseline back to
k-yoshimi/taskCover letter for an upstream contribution — what the baseline is, what changed, and how it was tested.
Prepared 2026-07-10. Full engineering detail:
docs/2026-06-TASK-merge-and-TRX-report.md.0. TL;DR for a reviewer
This proposes bringing the Kyoto
bpsi/developbase and the GitHub Phase-L modernization forkinto one reconciled, CI-green baseline. The merge is already done, resolved, and validated:
0254aa91— 1st parent58f1fe2e(Phase-L fork tip), 2nd parentfa9dd493(
bpsi/develop, +23 commits). i.e.bpsi/developmerged into the fork, keeping everymodernization and grafting bpsi's intent.
d1f7f9e7— 736 passed on Python 3.11 + 3.13; three1e-10equivalencebaselines regenerated on the CI compiler (gfortran-13.2), one of them for an owner-approved physics
refinement.
intent (table in §3).
Upstream already incorporated (§6): this branch merges current
k-yoshimi/developin (73 commits —mono-
libtotapirouting #201/#204/#208, the Linux-canonical equivalence policy #213, the Pythonfixture-parity backfill #215, and more) and adds the 57-commit bpsi baseline on top: 58 ahead / 0
behind. Only
.gitignoreneeded manual conflict resolution (unioned); the merge co-modifies just 5non-Fortran files (all clean unions) and no Fortran file, so nothing upstream is dropped. Offered as
a PR, never a force-push.
1. What the baseline is
Three git lines had drifted apart:
bpsi.nucleng.kyoto-u.ac.jp/pub/git/taskgithub.com/k-yoshimi/taskforklibXapi.so+ Python wrappers + MCP servers,trcommsplit into 6 sub-modulesgithub.com/ats-fukuyama/taskThe merge brings the latest upstream base onto the modernized fork without regression. An audit
established
bpsi/developis the latest base (not a 3-way upstream merge):ats-fukuyama/developis 119commits behind bpsi and its "unique" commits are superseded backups. So the merge is bpsi base → the
Phase-L fork.
2. What the contribution changes, in aggregate
Before this merge, the baseline was 57 commits ahead / 73 behind the current
k-yoshimi/develop(this PR incorporates all 73 — see §6, now 0 behind). The 57ahead touch 162 files (+9,637 / −5,522). The bulk is the bpsi merge itself (the
0254aa91mergealone reconciles 128 files) plus the CI-greening fixes and the design/plan docs. The net intent:
eq/trq-solver refinement (finerflux-surface grid,
EQLOOPunder-relaxation) used by themodelg=3eq_loadpath.libXapi.solayer, Python wrappers, MCP servers,and the 6-way
trcommsplit.3. Conflict resolutions (10 files)
Each conflict = keep fork modernization, graft bpsi intent:
eq/eqinit.f90NTVMAX200→400,NLPMAX20→100eq/eqcalc.f90EQLOOPadaptive under-relaxationeq/eqcalq.f90NPRINTprint +NMAX200→400eq/eqsub.f90pl/Makefilenoeqlibrecipes + take bpsiSRCS_NOEQ+plview.f90tr/trcomm.f90DEALLOCATE_TRCOMMtr/trcomm_param.f90PNM/PTM/PTPR/PTPP/PUM/PUPR/PUPP)tr/trmain.f90txnew/txcalv.f90wrx/wrcalpwr.f90dx4. How it was tested
4.1 The build had never been CI-green on the sandbox
Even the pure base
developfailed — the build never reached pytest. Getting to green surfaced issuesclass by class (auto-merge artifacts, a
PTsymbol clash betweenti/wm, apl_viewmodule move thatbroke
fpat link time, a repo-name assumption, and test-env gaps). Final:d1f7f9e7carries a two-reviewer pre-push sign-off marker.4.2 The
1e-10equivalence gateThe project keeps per-case regression baselines compared at a
1e-10tolerance. Three drifted onthe CI compiler (gfortran-13.2), for two distinct reasons:
wrx_demo/wrx_iter01— pure compiler-version FP reordering (gf8.5 ≠ gf13.2 ≠ Mac gf15, allwithin ~
1e-9). Lesson recorded:1e-10binary equivalence is too tight for FP-heavy modules acrosscompiler versions; baselines must be captured on the exact CI compiler. Regenerated on gf13.2.
tot_ht6m_short— a real ~1e-4physics shift (see §5), owner-approved and re-baselined.Because the CI env couldn't regenerate these directly, an env-gated capture (
REGEN_OUTPUT_DIR)dumps each case's freshly computed metrics as a CI artifact; those gf13.2 values were committed as the
new baselines.
4.3 Reproduce
5. The one deliberate physics change:
tot_ht6mq-solver refinementtot_ht6m_short(integrated eq→tr coupled run) shifted by ~1e-4. Root cause: bpsi's mergedEQ-solver refinements — finer flux-surface grid (
NMAX200→400) +EQLOOPunder-relaxation — on themodelg=3eq_loadpath. Signature (all 50 radial rows):jup in the core, down mid-radius to −7.9e-4 @ ρ=0.74, up at theedge;
qis its integral: down core, up outer);AJTΔ 3.9e-5 ≪ local Δ 7.9e-4);Reviewed and owner-approved as an intended higher-accuracy solve; the baseline was regenerated on
gf13.2 accordingly. (A truncated CI log initially mis-characterized this as core-localized; two
independent reviewers caught it against the full committed baseline — the write-up was corrected to
global redistribution.)
6. What this PR actually contains, and how it was validated
It is a merge, not a rewrite. The branch is
merge/upstream-reconcile-2026-07; its tip is a mergecommit whose parents are the delivered baseline (
c6cee542) and the currentk-yoshimi/develop(
4a5d977d). Result: 58 ahead / 0 behinddevelop.The merge is clean by construction — the two sides co-modify only 5 non-Fortran files, each a
verified clean union:
.gitignore,.github/workflows/python-tests.yml,python/eqlib/_ffi.py,python/totlib/tests/test_equivalence.py,python/wrxlib/tests/test_equivalence.py..gitignoreproduced a conflict marker (resolved as the union of both rule sets); the other fourauto-merged, and each was checked to keep both sides' additions (e.g.
_ffi.pykeeps oureq_save/psi_rzprototypes and upstream'sMONO_LIB_PATHrouting; the CI workflow keeps ournumpy/REGEN steps and upstream's
mono-buildjob).tot/, so the mergedtot(the mono-libtotapiwork, L-7b-ii Phase 2: monolithic mode + BPSD coupling activation (umbrella) #201/L-7b-ii follow-up: add Linux CI build job for libtotapi_mono.so #204/L-7b-ii Phase 2c: wrapper mono routing + activate ('eq','tr') BPSD coupling #208) is byte-identical to upstream's and known-good;and upstream's 73 commits changed no
tr/eq/pl/bpsdFortran, so those come wholly from thebaseline. There is therefore no semantic-merge risk in the Fortran.
Local validation (macOS, gfortran-15):
make -C tr tr2(static) builds from the merged sources;lib/mtxp/pl/eq/bpsdall compile.pytest --collect-only python/trlib python/totlibcollects 229 tests with no import/fixture breakage.Deferred to this PR's Linux CI — by upstream's own policy:
1e-10equivalence tests are now Linux-canonical (fplib + wrxlib equivalence tests fail 1e-10 tolerance on develop (mismatches ~1e-9) #213,@skipUnless(IS_LINUX)), so they donot run on macOS; the authoritative equivalence run is the Linux CI on this PR.
libtotapi_mono.sois a Linux-only build (L-7b-ii follow-up: add Linux CI build job for libtotapi_mono.so #204), so the mono routing/coupling tests likewise runon CI, not locally.
So the full 736-test suite (including the equivalence gate that this whole exercise protects) is
re-verified by the CI attached to this PR, against the merged tree.
7. Key commits