Repository navigation
Add POST /script/analyse for editor script validation - #717
Merged
Merged
Conversation
…-lsp
Frontend script editors need Python diagnostics that only a Mantid-aware
analysis service can give them, and the browser cannot hold a container. This
adds the FIA-API side of that path: an authenticated route that forwards a
script to mantid-python-script-lsp and hands the answer straight back.
- `AnalyseRequest` is the only model added; the response is forwarded as
unmodelled JSON with its status code intact, so a 503 from a cold service
stays a 503 and the editors can say "still starting up" instead of erroring.
- Rejections FIA-API makes itself (over-size, over rate limit) use the service's
own `{"detail": {"reason": ...}}` shape so the frontend parses one thing.
- The size cap counts UTF-8 bytes, not characters, to match the service's
`MAX_SCRIPT_BYTES`, and is refused before anything reaches Valkey or a worker.
- The per-user rate limit is a fixed window in Valkey, so it survives replicas
and restarts, and fails open when Valkey is down: an advisory endpoint should
not be able to take the editors down with it.
- Results are cached in Valkey under the script's SHA-256, and only a
well-formed 200 is stored, so a warm service cannot be read as cold for the
whole TTL. A cache hit echoes the caller's `content_hash`, which the cache
cannot know because results are shared across users.
- The upstream call has a 5 s timeout and runs off the event loop.
- `positive_int_from_env` is added to `core/utility.py` so a typo in a manifest
degrades to the default instead of crashing on import.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #717 +/- ##
==========================================
+ Coverage 97.05% 97.17% +0.12%
==========================================
Files 50 52 +2
Lines 2339 2442 +103
==========================================
+ Hits 2270 2373 +103
Misses 69 69 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The analysis settings were carried over as `*_ENV` plus `DEFAULT_*` name constants read through a helper, which is how the analysis service configures itself because that service must not crash at import. FIA-API reads config as a one-liner, and no other module here defines an env-name constant, so the extra globals were ceremony that made the settings harder to read rather than easier. - Each setting is now a single line, with the variable named in the call that reads it. All five stay overridable, so nothing new has to be redeployed by rebuilding. - `SCRIPT_LSP_URL` is read like `LLSP_API_HOST` already is, and `get_script_lsp_url` goes away; the trailing slash is still tolerated, since `//analyse` is a different path to anything downstream. - The tests for `positive_int_from_env` move to `test/core/test_utility.py`, where the function under test lives. Two fixes that only showed up against the versions `pyproject.toml` pins, which this environment had drifted behind: - `JWTAPIBearer` answers a request with no header at all with 401, not 403 (a present-but-invalid token is still a 403 from `authentication_error_handler`, which the test above the new one covers). - redis 8 types `incr` as returning `int`, so the cast added for an older redis-py stub is now a `redundant-cast` error under `mypy --strict`.
`positive_int_from_env` existed to keep a typo in a manifest from stopping the API starting. That is the wrong trade for a value like the size cap: a pod that comes up with a cap of nothing, or a rate limit of one, quietly disagrees with the analysis service about which scripts exist, and that is harder to notice than a pod that does not come up at all. - Each setting is now `int(os.environ.get(NAME, default))`, which is what `GITHUB_PACKAGES_CACHE_TTL_SECONDS` and the two cache TTLs in `routers/jobs.py` already do. - The helper and its tests are gone, and `core/utility.py` is back to what it was. - The effective settings are logged once at startup, since a value that did take effect is otherwise invisible until someone compares a rejection against a YAML file.
keiranjprice101
previously approved these changes
Nov 9, 2026
keiranjprice101
left a comment
keiranjprice101
left a comment
Collaborator
There was a problem hiding this comment.
This makes sense to me. Smart to cache on user number as my original instinct was "well there's only one per instrument anyway"
The line that prints what the process read from its environment never printed. It sat at module scope, module imports run before `logging.basicConfig` in `fia_api.py`, and a record emitted while there is no handler to receive it is dropped without trace -- `logging.lastResort` passes WARNING and above only. Found by running the built image and looking for it in the container log, where it was absent. This matters more here than for a typical informational line: a non-numeric `SCRIPT_ANALYSE_CACHE_TTL_SECONDS` is made to raise on import rather than fall back, so the log is where an operator confirms what a pod is actually running with, and those values are set in a manifest several repositories away from this one.
This was referenced Nov 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
** DO NOT MERGE UNTIL THE OTHER PARTS ARE MERGED**
Closes # N/A — no issue; this is the FIA-API slice of the script-editor validation work.
Description
Adds
POST /script/analyse, which forwards a script tomantid-python-script-lspand returns itsdiagnostics, so the script editors can show real Python errors instead of accepting anything until a
reduction fails several minutes later. See fiaisis/mantid-python-script-lsp#1
for the analysis service itself; a frontend PR follows this one and lands against it.
No issue to link: this is the FIA-API slice of the script-editor validation work.
Only
AnalyseRequestis modelled here. The response is forwarded as unmodelled JSON with its statuscode intact, because the frontend branches on the status: a
503from a cold service has to stay a503or the editors show a red error instead of "still starting up". Rejections FIA-API makes itself —
over-size, over rate limit — use the service's own
{"detail": {"reason": ...}}shape so the browserparses one thing whichever side refused.
The size cap counts UTF-8 bytes, not characters.
len(content)disagrees with the service'sMAX_SCRIPT_BYTESfor any script containing a non-ASCII character — a comment in Chinese or a unicodeem dash — and the two services refusing different scripts is worse than either cap being slightly
wrong. It is checked before Valkey or a worker is touched, and answered
413in the service's shape.The per-user rate limit is a fixed window in Valkey, so it survives replicas and restarts rather than
being per-pod memory. It fails open when Valkey is unavailable: this endpoint is advisory, and an
outage in the cache should not be able to take the editors down with it. 30 requests per 10 seconds per
user number leaves several times headroom over the frontend's 1 s debounce.
Only a well-formed
200is cached, underSHA-256(content)with a 10 minute TTL. Caching a503would have every editor read a warm service as cold until the entry expired. A cache hit has its
content_hashreplaced with the caller's own, because results are shared across users and the frontenddiscards a response whose hash does not match the text on screen — without that, every cache hit would
be thrown away as stale.
The upstream call has a 5 s timeout and runs off the event loop, so a wedged worker delays one
advisory request rather than every endpoint in the pod. FIA-API answers
504for its own timeout and503when the service is unreachable.New env vars are documented in
README.md:SCRIPT_LSP_URL,MAX_SCRIPT_BYTES,SCRIPT_ANALYSE_CACHE_TTL_SECONDS,SCRIPT_ANALYSE_RATE_LIMIT_MAX_REQUESTS,SCRIPT_ANALYSE_RATE_LIMIT_WINDOW_SECONDS. OnlySCRIPT_LSP_URLneeds setting per environment; thedefaults assume the in-cluster service DNS name.
positive_int_from_envis added tocore/utility.pyso that a typo in a deployment manifest falls backto the default with a warning instead of failing at import: these settings gate optional behaviour, and
none of them is a reason for the API to refuse to start.
Deployment note: nothing here needs a new public route —
/script/analyseis an ordinary FIA-APIendpoint, and
mantid-python-script-lspstays cluster-internal. Until that service is deployed,SCRIPT_LSP_URLresolves to nothing and the route answers503, which the frontend will treat as"validation unavailable" rather than an error.
33 new tests in
test/core/services/test_script_validation.py, including that the cap is bytes notcharacters, that a
503survives the trip, that a cached hit echoes the caller's hash, and that arejected JWT never reaches the analysis service.
ruff check/ruff formatclean,mypy --strict fia_apiclean, full unit suite green.