Repository navigation
feat(fp_mcp): expose set_param_str tool for KNAMEQ / MODELG=3 equilibrium load - #231
Conversation
…rium load
Every layer below MCP already supported string-valued fp parameters;
only the MCP server did not, so `KNAMEQ` was unreachable from any MCP
client:
* C ABI fp/fp_api.f90:155 — fp_api_set_param_str is
BIND(C, NAME="fp_set_param_str"), and the symbol is
present in the built library
(nm -gU fp/libfpapi.so | grep set_param_str ->
T _fp_set_param_str). No .so rebuild is required.
* registry fp/fp_param_registry.f90:171-182 — fp_param_set_str
has CASE ("KNAMEQ").
* wrapper python/fplib/fplib.py:163 — Fplib.set_param_str().
* MCP (this commit) — the gap.
This unlocks EQ->FP: eq_mcp's `save` writes an equilibrium file, and fp
can now be pointed at it with MODELG=3 + set_param_str("KNAMEQ", path),
the same file-handoff shape EQ->TR already uses. Without it, MODELG=3
falls back to the pl_init default KNAMEQ='eqdata', eq_load fails, and
BESEKNX trips with NCALC=-2 (see the comment block in
fp_param_registry.f90).
Changes mirror eq_mcp/server.py and tr_mcp/server.py:
* new `set_param_str` tool + handle_set_param_str() handler;
* `str` added to the SupportedValue union and to _apply_bulk_params,
so set_params({"KNAMEQ": "eq.bin"}) also routes correctly instead
of failing the float() coercion;
* KNAMEQ added to PARAMETER_REGISTRY as type "str";
* --print-tools, server instructions and docstrings updated (9 -> 10
tools).
Tests: two tests pinned the old numeric-only contract and were updated
to pin the new one (string -> set_param_str; 10 tools). Added coverage
for the registry entry, the handler, and the old-.so error mapping.
45 passed, 55 subtests passed.
Live-verified against the built libfpapi.so:
handle_set_param_str('KNAMEQ', 'eq.bin') -> ok
handle_set_param_str('KNAMFP', 'x.bin') -> rc=1 (registry has one CASE)
handle_set_params({'MODELG': 3, 'KNAMEQ': 'eq.bin'}) -> 2 parameter(s)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This has been green since 2026-08-06 and has had no review yet. Could one of you look at it when you have time? It is |
k-yoshimi
left a comment
There was a problem hiding this comment.
Reviewed at 62600919 (based on current develop tip 4a5d977d). Approving.
Verified locally (macOS, stock fp/libfpapi.so, pytest 9.1.1 --forked):
python/mcp-servers/fp_mcp: 45 passed (42 unit + 3 integration), also with the strict-subTest override from #243 active.- Claims cross-checked against
develop:Fplib.set_param_strexists with the rebuild-hintFplibError;fp_param_set_strhas exactly oneCASE ("KNAMEQ");nmshows 6 exportedfp_*symbols includingfp_set_param_str;_apply_bulk_paramskeeps the bool-reject first and the newstrbranch sits before the numeric one; the handler mirrorstr_mcp.handle_set_param_str. - Live probe:
set_param_str("KNAMEQ", "eq.bin")accepted,set_param_str("KNAMFP", ...)→FplibInvalidParamError rc=1, as the PR says.
One non-blocking note: fplib._encode_name(value, _MAX_C_STRING_VALUE_BYTES) caps the value at 80 bytes and raises FplibInvalidParamError beyond that; a one-line mention in the set_param_str tool docstring ("paths longer than 80 bytes are rejected") would save an LLM client a confusing round trip. Fine as a follow-up.
|
Independent Codex review (second reviewer, read-only) found no HIGH items. Code wiring stands as approved; three documentation points need a small fix before merge, all verified against
Non-blocking (follow-up material): the decorated Once 1–3 are pushed I will re-approve and we merge. |
…gives Three corrections asked for in the review of k-yoshimi#231, each checked against a build of this branch: - README 8.4 and the set_param_str docstrings no longer say the file is written by eq_mcp's `save` tool: develop has no such tool (it is on k-yoshimi#226). The file is a TASK/EQ-format equilibrium file, e.g. one written by EQSAVE or the eq program. - With MODELG=3, a KNAMEQ that names a file that does not exist or cannot be opened no longer "trips BESEKNX with NCALC=-2": fp_mesh returns eq_load's error and fp_run returns FP_ERR_CALC_FAILED. Measured for a missing file and for a directory: fplib raises FplibCalcFailedError (rc=3), and the handler turns it into the ToolError "calculation failed: fp_run(1): rc=3". The comment in fp/fp_param_registry.f90 describes the earlier behaviour and is left as it is. - README 8.3: MODELG=3 is the choice when an equilibrium file is to be loaded, not the default of iter01: fp_iter01.in sets no MODELG and runs with pl_init's MODELG=2. The same statement stood in two more places and is corrected there too: the example of 6.2 ("iter01-like parameters") set MODELG=3 with no KNAMEQ and failed with "calculation failed" as written (it now leaves MODELG out and runs), and the description of MODELG that describe_parameters serves said "3 = typical for iter01 fixture". Also documented: - KNAMEQ is CHARACTER(LEN=80), so a path of 81 to 128 bytes is cut to its first 80 characters without an error (measured: a 100-byte name loads the file that has its first 80 characters as its name), and the wrapper refuses more than 128 bytes. - A file that opens but is not a TASK/EQ file can end the process: eq/eqfile.f90 reads the leading records without IOSTAT (measured with an empty file and with a text file: "Fortran runtime error: End of file", exit status 2). That is older than this branch; the docstrings and the README now say it. No code path changes: the README, docstrings and one description string. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed
The statement of point 3 stood in two more places, corrected in the same commit:
Two things found while checking point 2, both older than this branch and documented here rather than fixed:
|
Answers the review of 411971c on k-yoshimi#256. Before merge, 1: only the first WRITE of EQSAVE was checked. What happens when a later write is refused was measured with a file size limit in a child process (macOS, gfortran 15; SIGXFSZ ignored, write() returns EFBIG), in a library loaded by Python: - a test routine, one 160 kB record in one transfer: a plain WRITE ends the process ("Fortran runtime error: File too large"), IOSTAT= returns the error. That is the case the review describes; - a test routine, records of 16 kB: IOSTAT is 0 on every WRITE, on FLUSH and on CLOSE, and the file is short; - EQSAVE itself, limits of 16, 1024, 4096 and 20000 bytes over nothing, an empty file and files of 8, 3000 and 45956 bytes (20 cases, on this commit): in 17 no statement reports anything and the file has the limit's length, of the 45956 bytes written; in the three with the 16-byte limit over a file that is not empty, the first WRITE, which had its IOSTAT, returns 27. On 411971c eq_save returned EQ_OK for a new file at each of the four limits. So EQSAVE_IERR does both. Every WRITE after the first and the CLOSE carry ERR=, for an error the run-time reports. And INQUIRE(UNIT=21, SIZE=) before the CLOSE, which gives the length written to the unit, is compared with INQUIRE(FILE=, SIZE=) after it; a shorter file returns 9 (when either length cannot be had, the comparison is left out). With gfortran the two are equal for a regular file that was written, also when a longer or a shorter file is overwritten (checked). The file name is asked of the unit, because with MODEFW 4 to 7 FWOPEN opens the name it read and not KNAMEQ. A device has length 0 and is reported short as well. Not measured: a full disk, and Linux (/dev/full does not exist here; the test for it runs where it does). What is at the path after such a failure is left there (deleting what the path names is not this routine's to decide). It is usually a short file, and it must not be loaded: run(1) with MODELG=3 on a file cut at 4096 bytes ends the process in EQRTSK, whose READs have no IOSTAT. That is outside this pull request; it is the item raised in the reply on k-yoshimi#231. The header, the docstrings and the README say so. Measured too: a save that leaves a short file leaves one descriptor open (+1 per save). Before merge, 2: EQSAVE is argument-less again. The work is in EQSAVE_IERR(IERR_OUT), which eq_save calls; EQSAVE calls it and drops the code. eq/eqmenu.f90 and tr/f77/trfile.f are as on develop. Suggestions taken: - eq_api.h and the README say that the "successful run" flag is what eq_api's own entry points saw, with examples of what bypasses it in the mono image. - The save tool of eq_mcp has overwrite (default false). It refuses an existing file with a message that names the argument (for a path that is not a regular file, a message that says that), and when it saves it sets MODEFW itself, 3 or 0. - Eq.save: the docstring says that KNAMEQ stays at the path once the path is accepted (eqlib has no getter to restore it from), and that a bytes path and a non-ASCII one are refused. - eq_api_set_param_str: a comment on why it leaves the flag alone. - eq_api.h no longer says eq_run(0) is not implemented; MODEFR and MODEFW are described in parameters.md (en, ja); the /tmp literal in test_eqlib.py is a relative name. Not taken: a line that eq_save is refused after tot_init until MODEFW is set. Measured in the mono image of this branch: after tot_init MODEFW is 0 and eq_run(0), eq_save write the file (rc 0). wr_api_init calls EQINIT after pl_init, and EQINIT sets MODEFW to 0. Tests (python/eqlib/tests/test_save.py, eq_mcp/tests/test_save_tool.py): a write refused at 16 and at 4096 bytes is EqlibCalculationFailedError with the "XX EQSAVE" line, the process alive and the file at the limit; a control without a limit; /dev/full where it exists; the argument-less EQSAVE writes the same bytes as eq_save (called with a null pointer, so that an EQSAVE that takes an argument ends the process instead of passing by accident); a bytes path and a non-ASCII path are refused and touch nothing; the tool refuses an existing file by default and leaves it unchanged, replaces it with overwrite, says "not a regular file" for a directory, and FWOPEN's mode 3 holds when a file appears after the tool's look. On the Fortran of 411971c four of these fail: the two refused-write cases (the save reports success) and the two that call EQSAVE without an argument (the process ends). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the approving review of 2026-10-18: 1. fp_mcp/server.py, the state schema: the scalar window is described with set_param only. This server has no set_param_str tool on this branch (k-yoshimi#231 adds it); the module's comment that fplib has no set_param_str is left for k-yoshimi#231, which edits that part of the file. 2. docs/fp-library/architecture.md and the same sentence of the fplib README: "String parameters not wired" was stale; fp_set_param_str (Fplib.set_param_str) sets KNAMEQ, and the other string keys stay at their defaults. 3. python/fplib/state.py and the docstring of test_state_scalars_window.py: the scalars are filled only while the state is prepared and no parameter was set since that preparation (was "from a prepared state only"). 4. CHANGELOG.md: k-yoshimi#253, the issue, as in the code comments (was k-yoshimi#259). 5. Two cases: a set the registry refuses leaves the scalars as they were (test_state_scalars_window.py); a set of tot's fp: parameters makes them read zero, on the mono image (test_set_after_run.py, the file CI's mono step runs; it checks the return code of both fp_get_state calls, since a failed read also leaves zeros). With the gate on g_prepared alone, the mono case fails (measured on a copy, fp and the mono image rebuilt). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
#233 and #240 are merged ( |
No line conflict. What the merge changes for this branch without one: - k-yoshimi#233 put a scalars object into fp_mcp's state schema, zero after a set until the next run; set_param_str opens that window too, which the schema text did not say yet (next commit). - k-yoshimi#256 brought eq_mcp's save tool, which this branch's docs still called future, after k-yoshimi#226 (fixed in a later commit). - k-yoshimi#232 and k-yoshimi#257 moved fp_mcp's fd isolation into python/_fd_isolation; this branch's server.py keeps it. - k-yoshimi#261 added the layout queries fp_state_abi_version and fp_state_size to libfpapi.so; fplib refuses a library without them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With develop merged in, this branch has both k-yoshimi#233's scalars object in the state schema and the set_param_str tool. The schema said the scalars are zero again after a set_param until the next run; a successful set_param_str opens the same window (fp_param_set_str moves the registry's generation counter, fp/fp_param_registry.f90:78, and fplib's test_state_scalars_window.py checks it for KNAMEQ), so the description says "after a set_param or set_param_str". Asked on k-yoshimi#231 and in the note on k-yoshimi#233. The module comment above SupportedValue is this branch's (str is in the union, set through set_param_str), as the merge kept it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With develop in this branch, a few sentences no longer hold: - eq_mcp's save tool is in develop (k-yoshimi#256), so "once k-yoshimi#226 lands" goes from the module docstring, the set_param_str tool docstring and README section 8.4. - The get_state tool docstring lists the returned keys and now names scalars too (the first LOW of the k-yoshimi#233 approval, left for this PR). - libfpapi.so exports eight C symbols since k-yoshimi#261: README section 2 now names the six entry points and the two layout queries fplib checks. - The Sphinx FP MCP pages (en, ja) still said nine tools and that the string setter is not reachable through MCP; they now list ten tools, with a set_param_str row, and point at README section 8.4. - fp_mcp/__init__.py's docstring said nine tools. - The comment above SupportedValue says that this server exposes the string setter as the set_param_str tool, as suggested on k-yoshimi#231. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tot_mcp builds its describe_parameters output from the sibling servers'
registries. Since this branch's set_param_str tool, fp_mcp's registry
has KNAMEQ as a string, so tot_mcp listed fp:KNAMEQ without saying
which of its tools can set it:
- set_param goes through tot_param_set_str (tot/tot_param_registry.f90),
which passes string values on for tr: and eq: only and returns
ierr=1 for fp:, ti:, wr: and wrx:. set_param("fp:KNAMEQ", ...) is
refused with rc=1 (measured in the review of this branch).
- run_pipeline's TotPipeline.set_param (totlib/pipeline.py) hands a
string to the module's own set_param_str, which eq, tr and fp have:
it accepts fp:KNAMEQ and refuses a ti: string (checked).
_as_tot_sees_it keeps the eq: and tr: registries as they are and
leaves out a ti:, wr: or wrx: string entry (there is none today). A
fp: string entry stays, with the first sentence of fp_mcp's
description (what the parameter is) followed by "Through tot_mcp: a
str value in run_pipeline's params only; set_param, set_params and
run_and_get_state refuse fp: strings (tot passes strings on for eq:
and tr: only)." fp_mcp's advice to use its set_param_str tool does not
apply to tot_mcp, which has no such tool, so it is not kept. Three
tests: tot_mcp's fp:KNAMEQ says this while fp_mcp's own entry is
unchanged, the rule per namespace on a synthetic registry (both fail
without this change), and eq:/tr: KNAMEQ stay strings. Passing fp:
strings through tot's set_param would be a Fortran change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s the run going Two points of the k-yoshimi#233 approval, offered for k-yoshimi#233's last push or this one, and not in k-yoshimi#233 as merged: - The raw-ctypes mono tests (python/totlib/tests/test_set_after_run.py) hand FpStateC and TiStateC to the image without the layout check the per-module wrappers do on load (k-yoshimi#246). _check_layout runs that same check (_abi_check.check_state_layout: layout version and size, with the mono image's rebuild hint) before the first call, in _drive_through_tot (the fp and ti re-preparation cases) and in the fp scalars case. With the expected version one higher, or the struct 8 bytes larger, all three fail. - test_a_refused_set_leaves_the_scalars observed the counter only through fp_get_state's gate. It now runs one more step and asserts that TIMEFP doubles: the run continues, with no re-preparation. With a successful set in place of the refused one, that assertion fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An Added entry, as k-yoshimi#232 has one for fp_mcp's fd isolation: the tool, what it calls and needs, the scalars window it opens, and that tot_mcp lists fp:KNAMEQ as run_pipeline only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your LOW 2 on #233 (README §8.4) was this PR's §8.4 rewrite already. Of #273, this PR settles the Locally on the head (macOS, gfortran 15, a clean build of the merged tree; the later commits change no Fortran; CI's flags |
k-yoshimi
left a comment
There was a problem hiding this comment.
Review of the delta after the merge of develop (557344c8..08d60044), through the full cycle (Codex + in-house correctness and test-oracle lenses + docs/workflow review), with the tests run under the CI flags.
Verdict: approve. The merge commit's tree is identical to a clean merge of develop ea9806ac into 9cd01e66, and the five commits after it change no Fortran, so develop's libfpapi.so, libtotapi.so, libtrapi.so and mono image are the right binaries for this head; I used them. Locally: fp_mcp + tot_mcp + fplib's scalars-window test, 133 passed / 5 skipped (the skips want libtotapi.so in the tree); with develop's libtotapi.so and libtrapi.so placed in the worktree and MONO_LIB_PATH on the mono image, test_set_after_run.py and tot_mcp's integration tests 14 passed, no skip. CI on 08d60044 is green.
What the reviewers verified: tot/tot_param_registry.f90:124-134 forwards strings for tr and eq only, and totlib/pipeline.py:504-511 routes a string to the module's own set_param_str, which eqlib/fplib/trlib have, so the fp:KNAMEQ description _as_tot_sees_it writes is true on both halves. The string entries today are eq: KNAMEQ/KNAMWR/KNAMWM and tr:/fp: KNAMEQ, none in ti/wr/wrx, so the "left out" branch is exercised only by the synthetic test. PARAMETER_REGISTRIES consumers (_flat_count, describe_parameters, the shape tests) see the same counts, order and, for eq/tr, the same dict objects. _check_layout calls check_state_layout with its real signature, both *_state_abi_version/*_state_size pairs are in the mono image (whole obj/pic wildcards, no export list), and the MONO_LIB_PATH skip is in setUp. The TIMEFP assertion discriminates: fp_run re-preps only when the generation moved (fp/fp_api.f90:236-244), the registry bumps only on ierr==0, and fp_prep zeroes TIMEFP, so a spurious re-prep would give DELT, not 2*DELT. Counts: 10 @mcp.tool() in fp_mcp/server.py, 8 BIND(C) names in fp/fp_api.f90.
LOW (no re-review needed if you take them, a push is enough).
tot_mcp/server.py:225:.split(". ")[0]cuts a first sentence at "e.g. " or "i.e. ". The one entry it is applied to has none, so today's output is right, but the test asserts only a prefix; pinning the exact part before ". Through tot_mcp:" would catch a future reword offp_mcp's description._PIPELINE_STRING_NAMESPACESlistseqandtr, which return at line 216-217 and never reach that branch; onlyfpis live.docs/sphinx/modules/fp/en/mcp.mdsends the English reader to section 8.4 of a README written in Japanese._check_layouthard-codeslib_name="libtotapi_mono.so"while the path comes fromMONO_LIB_PATH.tot_mcp'sset_param/set_params/run_pipelinedocstrings could point anfp:KNAMEQcaller atrun_pipeline'sparams, and the "no eq_mcp yet" comment at:122-125/:140-141is stale.
Pre-existing, not on this PR. The docs review found that the tot Sphinx pages (parameters.md, parameter-setting.md, en and ja) show tot.set_param_str("fp:KNAMEQ", …) and "wr:KNAMEQ", which tot_param_set_str refuses with rc=1, that tot_mcp/__init__.py and the tot Sphinx pages say 9 tools where server.py registers 10 (run_pipeline has no row), and that _wrap_totlib_error's "names must be namespaced" hint misleads when a correctly namespaced fp: string is refused. All of that is on develop unchanged and is now #274. The fplib README and the fp/fp_api.f90 header still naming five symbols belongs to #273.
Merge on the maintainers' instruction; after #267 in the queue, since both touch the mono tests.
- _as_tot_sees_it took the first sentence with split(". "), which cuts
at "e.g. " or "i.e. "; it now splits at a period followed by a
capital, so an abbreviation before a lowercase word no longer ends
it. The test pins the exact first sentence of fp:KNAMEQ, and the
synthetic case has an "e.g." (it fails with the old split).
- _PIPELINE_STRING_NAMESPACES listed eq and tr, which return before
that branch; it is _PIPELINE_ONLY_STRING_NAMESPACES = ("fp",).
- The docstrings of set_param, set_params and run_pipeline point a
fp:KNAMEQ caller at run_pipeline's params; the two "no eq_mcp yet"
comments say that the inline eq: registry predates eq_mcp.
- The English Sphinx FP MCP page says what set_param_str is for instead
of pointing at a section of the Japanese README.
- _check_layout names the image it loaded (the basename of
MONO_LIB_PATH) in its rebuild hint, not a fixed libtotapi_mono.so.
No Fortran changes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The LOWs are in
Locally: |
…t, the refused-string hint (#274) The four items of #274, all on develop as found by the docs review of #231, and the stale stub notes of tot_mcp, totlib and the tot docs: 1. The tot Sphinx pages (parameters.md and parameter-setting.md, en and ja) called tot.set_param_str on "fp:KNAMEQ" and "wr:KNAMEQ". tot_param_set_str (tot/tot_param_registry.f90) passes strings on for eq: and tr: only and returns ierr=1 for the others (checked with libtotapi.so: eq:KNAMEQ and tr:KNAMEQ accepted, fp: and wr: refused with rc=1). The examples use eq: and tr:, and say that fp:KNAMEQ goes through totlib.TotPipeline.set_param, which hands it to fplib's own set_param_str (tot_mcp: run_pipeline's params), and that ti, wr and wrx take no strings. They no longer say that each module reads its own KNAMEQ: eq's registry and fp's both write plcomm's KNAMEQ (tr has its own in trcomm_ctrl), so where eq and fp share one copy of pl, as in libtotapi_mono.so (one plcomm_parm_MOD_knameq), eq:KNAMEQ also names the file fp loads. 2. tot_mcp registers 10 tools; __init__.py and the Sphinx MCP pages said 9, and the tables lack run_pipeline. They say 10 and have the row. 3. tot_mcp/README.md 6.4 names the run_pipeline path for fp:KNAMEQ; the set_params row of section 7 lists strings (eq: and tr: only). 4. _wrap_totlib_error told every refused parameter to namespace its name, also a correctly namespaced string that tot refuses. The hint now follows the refused call: - tot_set_param_str: a hint that names the string parameters (eq: KNAMEQ, KNAMEQ2, KNAMWR, KNAMWM, KNAMFP, KNAMFO, KNAMPF; tr: KNAMEQ), says to send numbers as numbers (a number sent as a JSON string takes this path) and gives the run_pipeline path for fp:KNAMEQ; - tot_set_param: the name passed totlib's namespace check, so the hint says the module does not know the name or refused the value, and points at describe_parameters; - a string totlib rejects itself (not ASCII, a NUL, too long) or another call's refused argument (tot_run(-1)): no hint; - anything else, e.g. totlib's namespace check: the namespace hint. The call is matched at the start of the message only, so a value that contains "tot_set_param_str(" is not mistaken for one. A message that ends in a period no longer gets ".." before the hint. Six tests; five fail on develop, the sixth pins the namespace hint that stays. 5. Several texts said that run and get_state are stubs until L-6, or that tot_mcp has a set_param_str: the tot_mcp server instructions, the run and get_state docstrings, the module docstring and the rc=4 message; tot_mcp/README.md 4, 6.1, 7 and Q5; totlib's errors.py, README and the three examples (two printed it); and docs/tot-library/architecture.md (its known limitations, the rc=4 row, the phase table and the acceptance criteria, which had L-6 pending). L-6 is in (#96, #124): test_integration.py drives init, run, get_state and finalize; no Fortran entry point returns TOT_ERR_NOT_IMPLEMENTED; a libtotapi.so built before #246 is refused at load (TotlibAbiError), so rc=4 cannot reach a caller now. tot_mcp has no set_param_str tool. The texts now say what the entry points do: init/finalize bring up and tear down eq (since #209), tr, ti, fp and wr; run advances TR; get_state fills TR's slots (also corrected in totlib's README and module docstring, which left out eq). architecture.md also says, in its quirks and its limitations, that fp has a string setter of its own, reached through TotPipeline, not tot; that totlib refuses a second live Tot() in a process (it said the two would share state; so did totlib's README and the Tot docstring, corrected too); and that a library from before L-3 is refused at load (it said one would load). The comments of totlib's _apply_prototypes and Tot._open, its README and its module docstring call their pre-L-3 and pre-L-6 allowances leftovers. Left as they are: the comments of tot/tests/c_abi/test_negative.c and of tot/tot_param_registry.f90 (no Fortran or C change here). And a CHANGELOG Fixed entry, after the third one, where no other open pull request inserts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
k-yoshimi
left a comment
There was a problem hiding this comment.
bfa64d9e checked (Codex pass on the code hunks + the tests run here). Diff against 08d60044: tot_mcp/server.py (regex first-sentence split, _PIPELINE_ONLY_STRING_NAMESPACES, docstrings, the two stale comments), its tests, _check_layout's lib_name, the English Sphinx page. No Fortran.
What was checked: the regex \.\s+(?=[A-Z]) gives exactly the first sentence of the real fp:KNAMEQ description (the test now pins it), keeps "e.g. a" whole, preserves a description with no period, and the template restores the period rstrip removes; fp: is the only namespace reaching that branch, so no other description is transformed. ctypes.CDLL._name is the path given to CDLL on 3.11 and 3.13 and os is imported in the test module. Nothing else references the removed _PIPELINE_STRING_NAMESPACES. Codex: no must_fix / should_fix; one optional (more regex edge cases in the synthetic test, e.g. "e.g. ASCII"), not needed for any entry that exists.
Locally at this head, with develop's libfpapi.so / libtotapi.so / libtrapi.so and MONO_LIB_PATH on the mono image: fp_mcp + tot_mcp + fplib scalars window + test_set_after_run.py, 144 passed, no skip. CI 3/3. Merges clean against develop, #267, #269, #270 and #226.
Verdict: approve (renewed on this head). Merge on the maintainers' instruction; #274 as its own PR is fine.
Summary
Exposes
set_param_stras anfp_mcpMCP tool. This is pure wiring — every layer below the MCP server already supports it:fp/fp_api.f90:165fp_api_set_param_str…BIND(C, NAME="fp_set_param_str")— already an exported symbolfp/fp_param_registry.f90:73-79,:295fp_param_set_str(the wrapper that counts a successful set, #253) and itsCASE ("KNAMEQ")python/fplib/fplib.py:163Fplib.set_param_str()python/mcp-servers/fp_mcp/server.pyNo Fortran changes: a
libfpapi.sobuilt fromdevelopalready exports the symbol (fplibrefuses a library whosefp_state_tlayout is older thandevelop's version 2):Why
fp_param_registry.f90's own comment states the intent (written beforefp_prepreturned the load error; today a missing file stops atfp_meshwithFP_ERR_CALC_FAILED, see Verification):Until now that entry point was unreachable from an MCP client, so
fp_mcpcould take theMODELG=3equilibrium-load path only withpl_init's defaultKNAMEQ='eqdata'in its working directory: a client could not name the file.eq_mcpandtr_mcpboth already exposeset_param_str; this bringsfp_mcpto parity.Changes
stradded to theSupportedValueunion, soset_params({"KNAMEQ": ...})routes to the string setter instead of dying infloat()KNAMEQregistered inPARAMETER_REGISTRYastype: "str"handle_set_param_str()handler, error-mapped through the existing_wrap_fplib_error(FplibError→ rebuild hint, registryrc=1→invalid parameter)--print-tools(9 → 10 tools), instructions string, docstringsREADME.md: tool table,--print-toolsoutput, and two corrections — §8.3 describedMODELG=3as "analytic" when it is the file-loading path, and §8.4 said string parameters were unsupportedAdded on 2026-10-21, after
develop(ea9806ac, with #232, #233, #240) was merged in:scalarsdescription in the state schema: zero again "after a set_param or set_param_str"; theget_statedocstring namesscalars(feat(fp): expose global plasma scalars (Ip, stored energy, powers, volume) in fp_state_t #233's first LOW)eq_mcp'ssavetool is indevelop, no longer "once Reconcile bpsi/develop base into develop (bpsi↔fork merge baseline, CI-green) #226 lands"; the Sphinx FP MCP pages (en, ja) list ten tools with aset_param_strrow instead of saying it is not exposed; README §2 names the eight exported symbolstot_mcplistsfp:KNAMEQasrun_pipelineonly: it readsfp_mcp's registry, but tot'sset_parampasses string values on foreq:andtr:only (tot/tot_param_registry.f90;set_param("fp:KNAMEQ", ...)is refused with rc=1), whilerun_pipelinesets it through fplibTIMEFPdoubles (no re-preparation)CHANGELOG.mdAdded entryVerification
Against a stock build (
gfortran 15.2.0, macOS arm64):set_param_str("KNAMEQ", "eq.bin")accepted;set_param_str("KNAMFP", ...)correctly rejected withrc=1(the registry has exactly oneCASE)eq_mcp'ssavetool is loaded by FP —# OLD FILE (eq.bin) IS ASSIGNED FOR INPUT., andeqgetbwrites the file's geometry back into FP (DEVICE, RR, RA, BB = 3.60E+00 1.20E+00 4.50E+00, matching the perturbed EQ run rather than FP's own defaults). Mesh metrics move accordingly (rsrhon0.805→1.0264,bpm0.3506→0.1829,ql2.2961→7.0134), and global integrals scale with the volume ratio (stored energy 5.369→10.341 MJ = 1.93× vsTVOL28.42→53.12 = 1.87×)KNAMEQat a missing file gives this chain (the registry comment predictedBESEKNXtripping withNCALC=-2; the load error now stopsfp_prepfirst) —XX FROPEN (EQ): FILE NOT FOUND→eq_load: eqload: ierr=7→XX FPMESH:EQLOAD:IERR=7→XX fp_prep: fp_mesh faileddevelop: 45 passed, 55 subtests. Two existing tests pinned the old contract (test_rejects_string_value,test_print_tools_lists_nine) and were rewritten to pin the new one; added coverage for the registry entry, the handler, and the error mapping for a simulated missing-symbol errorNote for reviewers
#232 (the stdout guard) is merged;
fp_mcp's fd isolation isdevelop'spython/_fd_isolation.py, kept as it is by the merge.One finding worth recording, though it needs no change here: FP's six
get_stateprofile arrays are local per-unit-volume moments, so withMODELD=0and no wave/current-drive model they are bit-identical across different equilibria. The equilibrium enters only through the volume element and mesh metrics, whichfp_state_tdid not expose. The geometry-dependent globals the code already computes (rtotalIP, stored energy, total collision/absorption power,TVOL) were written to unit 6 but not returned in the state. #233 now returns seven of them asscalars, and a successfulset_param_strzeroes them until the next run, as a numeric set does.🤖 Generated with Claude Code