Skip to content

fix(fmsr): take the FMSR model from FMSR_MODEL_ID, with no built-in default - #583

Merged
ShuxinLin merged 5 commits into
harbor-private-mount-docker-mdfrom
fmsr-model-pin
Sep 30, 2026
Merged

ShuxinLin merged 5 commits into
harbor-private-mount-docker-mdfrom
fmsr-model-pin

Conversation

@DhavalRepo18

Copy link
Copy Markdown
Collaborator

fix(fmsr): take the FMSR model from FMSR_MODEL_ID, with no built-in default

Base: harbor-private-mount-docker-md (at 460543f) · Head: fmsr-model-pin
Ported from: main_fmsr_llm (1b28baa), FMSR changes only, with the watsonx
fallback dropped rather than retained.

The bug

generate_failure_modes calls an LLM inside the FMSR MCP server. The model came
from FMSR_MODEL_ID, and with that unset the server fell back to
watsonx/meta-llama/llama-3-3-70b-instruct. Nothing ever set it, so every run
needed WATSONX_APIKEY and WATSONX_PROJECT_ID regardless of which model was
being benchmarked:

ERROR:fmsr-mcp-server:generate_failure_modes failed: litellm.BadRequestError:
WatsonxException - {"errorCode":"BXNIM0415E",
"errorMessage":"Provided API key could not be found."}

Found on a mini-suite sweep: five trials hit it, fmsr-902 four times and
fmsr-913 once. The tool was dead for the whole trial, and the error named
neither the model nor the missing variables, so the cause was invisible from the
trajectory.

The fix

FMSR_MODEL_ID is the only source, and there is no default. A hardcoded
default sent every run at one provider and demanded that provider's credentials
whatever model was under test. That is the bug, not a convenience, so it is gone
rather than changed to a different vendor.

Agent runners pin FMSR_MODEL_ID for the MCP servers they spawn. An explicit
value from the shell or .env wins; otherwise the agent's own --model-id. The
value is always set, even when it equals the agent model, so the FMSR model is
fixed and visible for every run. A normal run needs no extra configuration and
the FMSR tools use the same model and credentials as the agent under test.

src/agent/runner.py holds three helpers:

resolve_fmsr_model_id(agent_model_id)  # explicit > agent model > None
fmsr_env_overrides(agent_model_id)     # {FMSR_MODEL_ID: ...} or {}
mcp_server_env(agent_model_id)         # {**os.environ, **overrides}

Each runner calls one where it builds its stdio server specs: Stirrup, deep,
openai and plan-execute take the full mcp_server_env (the MCP SDK otherwise
passes only HOME/PATH/SHELL to children); claude and opencode already
forward a parent environment and take fmsr_env_overrides on top.

A server started standalone with FMSR_MODEL_ID unset says so instead of
guessing:

LLM unavailable (FMSR_MODEL_ID is not set; the generate_* tools need a model id,
e.g. tokenrouter/<model> or litellm_proxy/<provider>/<model>)

_build_llm takes the model as an argument and keeps its per-prefix credential
checks, so an explicit watsonx model still reports the right missing variables
for anyone who has it. Empty values count as unset rather than as a model id.

Why the Harbor path needs nothing extra

_build_mcp_config in the Stirrup runner already forwarded the parent
environment so the servers could see COUCHDB_URL and the *_DBNAME overrides.
mcp_server_env keeps that and adds the pin, so the one call site covers both
the CLI and the Harbor adapter. CREDENTIAL_ENV_VARS in
assetops_harbor/stirrup.py is untouched.

Verification

FMSR_MODEL_ID unset (standalone)
  model : None
  usable: False
  error : FMSR_MODEL_ID is not set; the generate_* tools need a model id, ...

a tool call in that state
  error='LLM unavailable (FMSR_MODEL_ID is not set; ...)'

pinned by the runner, no other provider's credentials present
  model : tokenrouter/MiniMax-M3
  usable: True

The Stirrup MCP config carries the pin without losing the environment forwarding:

FMSR_MODEL_ID  -> tokenrouter/MiniMax-M3
COUCHDB_URL    -> http://couchdb:5984
explicit wins  -> litellm_proxy/aws/claude-opus-5

246 tests pass across src/agent, src/servers/fmsr and
src/assetops_harbor (1 skipped, the watsonx-gated live call). ruff reports
one finding fewer than the base branch on the touched files.

Docs

docs/running_benchmark.md loses its "WatsonX — needed more often than it looks"
section, since WatsonX is no longer implicated, and gains one describing how the
model is pinned. The troubleshooting row now points at the model named in the
error rather than at absent WatsonX credentials. INSTRUCTIONS.md and
.env.public state that there is no default.

Two notes for review

FMSR_MODEL_ID is not in CREDENTIAL_ENV_VARS, so a host-side override does not
reach the agent container through Harbor. It is not needed for the fix, since the
in-container runner derives the value from --model-id, but add it there if you
want to pin a single FMSR model across a Harbor sweep.

src/servers/fmsr/tests/test_tools.py still gates its one live-LLM test on
requires_watsonx, so that test now always skips. Generalising it to whatever
FMSR_MODEL_ID names would be a good follow-up; I left it alone to keep this
diff to the fix.

Not ported

main_fmsr_llm sits on an older main, so a plain diff against this branch shows
thousands of unrelated deletions (the whole benchmarks/harbor/ tree,
assetops_harbor/, uv.lock, pyproject.toml). This PR carries only the files
that commit actually changed, plus the doc updates the removed default requires.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q7W9hWwuZU8Mkwo3ZCrR7m

…efault

generate_failure_modes calls an LLM inside the FMSR MCP server. The model came
from FMSR_MODEL_ID, and with that unset the server fell back to
watsonx/meta-llama/llama-3-3-70b-instruct. Nothing set it, so every run needed
WATSONX_APIKEY and WATSONX_PROJECT_ID regardless of which model the agent was
being benchmarked on, and without them the tool was dead for the whole trial:

    ERROR:fmsr-mcp-server:generate_failure_modes failed:
    litellm.BadRequestError: WatsonxException - {"errorCode":"BXNIM0415E",
    "errorMessage":"Provided API key could not be found."}

Observed on five trials of a mini-suite sweep (fmsr-902 four times, fmsr-913
once), and the error text named neither the model nor the missing variables, so
the cause was not visible from the trajectory.

FMSR_MODEL_ID is now the only source for that model, and there is no default at
all. A hardcoded one sent every run at a single provider and demanded that
provider's credentials whatever model was under test, which is the bug rather
than a convenience.

Agent runners pin FMSR_MODEL_ID for the MCP servers they spawn: an explicit
value from the shell or .env wins, otherwise the agent's own --model-id. The
value is always set, even when it equals the agent model, so the FMSR model is
fixed and visible for every run. src/agent/runner.py holds the three helpers;
each runner calls one where it builds its stdio server specs. A normal run
therefore needs no extra configuration and the FMSR tools use the same model and
credentials as the agent under test.

A server started standalone with FMSR_MODEL_ID unset reports the generate_*
tools as unavailable and says why, rather than picking a provider:

    LLM unavailable (FMSR_MODEL_ID is not set; the generate_* tools need a
    model id, e.g. tokenrouter/<model> or litellm_proxy/<provider>/<model>)

_build_llm takes the model as an argument and keeps its per-prefix credential
checks, so an explicit watsonx model still reports the right missing variables.
Empty values count as unset rather than as a model id.

The Stirrup runner already forwarded the parent environment so the servers could
see COUCHDB_URL and the *_DBNAME overrides; mcp_server_env keeps that and adds
the pin, so the Harbor path is covered by the same change.

Docs follow: running_benchmark.md loses its "WatsonX - needed more often than it
looks" section, since WatsonX is no longer implicated, and the troubleshooting
row now points at the model named in the error.

Verified: unset resolves to no model and the tools report the cause; pinned to
tokenrouter/MiniMax-M3 the backend builds with no other provider's credentials
present. The Stirrup MCP config carries FMSR_MODEL_ID=tokenrouter/MiniMax-M3
alongside COUCHDB_URL, and an explicit FMSR_MODEL_ID overrides the agent model.
246 tests pass across src/agent, src/servers/fmsr and src/assetops_harbor; ruff
reports one finding fewer than the base branch on these files.

Ported from main_fmsr_llm (1b28baa), FMSR changes only, with the watsonx
fallback dropped rather than retained.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q7W9hWwuZU8Mkwo3ZCrR7m
We do not run WatsonX, so the FMSR server should not carry a half-supported
path for it. _build_llm now takes its accepted prefixes and their credential
variables from llm.routers.PROXY_ROUTERS, which already declares itself the
single source of truth for router prefixes and lists exactly litellm_proxy/ and
tokenrouter/. The hand-written per-prefix branches are gone, so the two lists
cannot drift and a new router is added in one place.

A model id with no known prefix is now rejected rather than pushed at LiteLLM on
a guess, which the old else-branch did:

    FMSR_MODEL_ID='watsonx/...'  -> no supported router prefix (litellm_proxy/<model>, tokenrouter/<model>)
    FMSR_MODEL_ID='gpt-4o'       -> no supported router prefix (...)
    tokenrouter/ without keys    -> Missing env vars for tokenrouter: [...]
    tokenrouter/ with keys       -> OpenAICompatBackend
    litellm_proxy/ with keys     -> LiteLLMBackend

The FMSR test gate followed: requires_watsonx skipped on WATSONX_APIKEY, so the
one live-LLM test never ran once WatsonX went away. requires_fmsr_llm now gates
on whether the configured FMSR model is actually usable, so the test runs
whenever FMSR_MODEL_ID and that router's credentials are set.

247 tests pass across src/agent, src/servers/fmsr and src/assetops_harbor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q7W9hWwuZU8Mkwo3ZCrR7m
Credentials already reached the FMSR server: _credential_env forwards every
non-empty CREDENTIAL_ENV_VARS entry unfiltered by prefix, so both routers' keys
land in the container, and mcp_server_env passes the whole parent environment to
the spawned servers. The default case was therefore fine, because FMSR runs on
the agent's own model and those credentials are present by definition.

FMSR_MODEL_ID itself was not forwarded. An operator who exported it on the host
had it dropped at the container boundary, and the in-container runner then set it
to the agent's --model-id, so the documented "explicit value wins" held for the
CLI and silently did not for Harbor. SETTING_ENV_VARS carries it across as a
setting rather than a credential.

That makes a differently-routed FMSR model reachable, so _require_router_credentials
now checks FMSR_MODEL_ID alongside --model-id. An agent on tokenrouter with FMSR
pinned to litellm_proxy and no LITELLM_* present used to build an image and a
container per trial before failing at the first generate_failure_modes call; it
now fails before Harbor builds anything, naming which of the two model settings
needs the variables.

FMSR_MODEL_ENV is a literal here for the same reason ROUTER_CREDENTIALS is a copy:
importing agent.runner would pull the agent SDKs into the host-side Harbor
process. A test asserts it tracks agent.runner.FMSR_MODEL_ENV.

Verified end to end. Default: FMSR model tokenrouter/MiniMax-M3 with both
TOKENROUTER_* present. Operator pins litellm_proxy/aws/claude-opus-5: that model
arrives in the server env with both LITELLM_* present. 250 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q7W9hWwuZU8Mkwo3ZCrR7m
requires_fmsr_llm replaced requires_watsonx so the one live generate_failure_modes
test would run again once a model was configured. It then ran on a machine with no
CouchDB and failed:

    assert 'generated' in {'error': "HTTPConnectionPool(host='localhost', port=5984)
    ... Connection refused"}

The test needs the LLM and the failure_mode database. The old watsonx gate hid
that by skipping for an unrelated reason, so the missing database requirement was
never expressed.

requires_failure_mode_db probes fm_db with a real query, mirroring
_couchdb_reachable in the iot server's conftest, so a configured-but-down CouchDB
skips rather than fails. The integration test carries both markers.

Verified with no CouchDB running: 22 passed, 1 skipped, the skip naming the
database. 250 tests pass across src/agent, src/servers/fmsr and
src/assetops_harbor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q7W9hWwuZU8Mkwo3ZCrR7m
generate_failure_modes documents the behaviour it did not have:

    If no stored modes exist, the LLM generates a new list from scratch.

_known_failure_modes raises when there is no catalog to read, and that
propagated out as an ErrorResult, so the documented from-scratch path was
unreachable. On the mini suite this bit hardest where it mattered: the scenarios
whose manifests never load failure_mode are exactly the ones where models
reached for generate_* after get_failure_modes returned 'database not
connected', and the generate call failed for the same reason.

The stored catalog is now optional context. _optional_known_failure_modes
returns ([], note) when there is no catalog to read and propagates everything
else, so the four states separate:

    CouchDB unreachable                  -> generates, 'catalog unreachable'
    fm_db unset                          -> generates, 'catalog not initialised'
    server up, database never created    -> generates, 'catalog not initialised'
    catalog exists but the read fails    -> ErrorResult

The last one stays an error deliberately. Stored modes may exist and be
temporarily unreadable, and generating a list that quietly ignores them would
hide a real fault behind plausible output. TestGenerateFailureModes::
test_database_read_error_returns_error already asserted this and still passes;
it is the reason the fix distinguishes absent from unreadable rather than
treating every database failure as empty context. The result message names
which state applied, so a run can be read back later.

get_failure_modes and add_failure_modes are untouched: a read tool with no data
has nothing to return, and a write needs somewhere to write.

Also strips markdown emphasis from generated mode names. kimi-k3 answers
'**Cavitation**' and the parser stripped one leading asterisk, producing
'*Cavitation**' as the mode name. Emphasis is removed before bullets, because
'*Cavitation*' is italic rather than a bullet plus a stray asterisk, and again
after, for '- **Bearing wear**'. Nobody had seen this because the tool never ran.

252 tests pass, 2 new.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q7W9hWwuZU8Mkwo3ZCrR7m
@ShuxinLin
ShuxinLin merged commit b83e062 into harbor-private-mount-docker-md Sep 30, 2026
5 checks passed
ShuxinLin added a commit that referenced this pull request Sep 30, 2026
fix(fmsr): take the FMSR model from FMSR_MODEL_ID, with no built-in default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants