Repository navigation
feat(test): add Tier 1 safety judges for the confirm gate and personas - #346
abhishekKokadwar wants to merge 4 commits into
Conversation
ARCHITECTURE.md lists rule-based safety judges as Tier 1 of the eval pipeline, run on every PR. This adds them as plain pytest under tests/eval, with no cluster, no LLM and no new dependency. The per-tool unit tests check each preview one tool at a time. These judges check the rules that hold across all tools, through an in-memory MCP client: - a tool is read-only exactly when it has no confirmed parameter, and confirmed defaults to False - confirmed=False returns a preview and sends no write request, for every mutating tool, with a control that confirmed=True does write - each persona exposes exactly its allowlist, and hidden tools cannot be called - the same checks through execute_tool in progressive and semantic modes - every mutating tool must be listed in GATE_ARGS, so a new tool fails the suite until it is covered Kubernetes is faked at ApiClient.call_api, which both the SDK and the raw clients go through. No tool name, schema or response changes. Signed-off-by: Abhishek <abhikokadwar2@gmail.com>
The judges built the server with the default client list, which is trainer only. On the Spark branch (kubeflow#51) they did not look at the Spark tools at all, and the persona check failed because the policy lists Spark tools that a trainer-only server does not expose. Build every server with all of CLIENT_MODULES. A new client's mutating tools are then checked without editing the test file, and the suite fails until their arguments are added to GATE_ARGS. Nothing changes on main: 41 checks pass, since the optimizer and hub stubs register no tools. Signed-off-by: Abhishek <abhikokadwar2@gmail.com>
|
/assign |
pratik-naik003
left a comment
pratik-naik003
left a comment
There was a problem hiding this comment.
Thanks @abhishekKokadwar, really useful checks. I ran them locally: all 41 pass, and they catch real breaks. Making update_training_job patch on preview fails 3 checks, and bypassing the persona filter fails 13. Two small non-blocking nits inline.
/lgtm
| async def test_persona_exposes_exactly_its_allowlist(persona): | ||
| exposed = set(await _list_tools(persona)) | ||
| allowed = get_allowed_tools(persona) | ||
| if allowed is None: |
There was a problem hiding this comment.
Nit: for platform-admin this returns without checking anything, so that case always passes. Could it assert that every registered tool is exposed, or use pytest.skip so it's visible?
There was a problem hiding this comment.
Agreed, that case checks nothing as written. I will make it assert that platform-admin sees every registered tool. Since this has lgtm I would rather not push and reset it, so I will send that as a small follow-up right after merge, unless you would prefer it here.
|
|
||
| from kubeflow_mcp.core.policy import get_allowed_tools | ||
| from kubeflow_mcp.core.server import CLIENT_MODULES, create_server | ||
| from tests.eval.conftest import GATE_ARGS, payload |
There was a problem hiding this comment.
Nit: importing from conftest.py is usually avoided, since pytest loads it itself and it can end up imported twice. Moving GATE_ARGS and payload to a small helper module would be cleaner.
There was a problem hiding this comment.
Good point. I will move GATE_ARGS and payload into a small helper module in the same follow-up.
|
/assign @abhijeet-dhumal |
|
/ok-to-test |
|
/assign @jaiakash |
|
@jaiakash when you have a moment, could you take a look for /approve? Tests only (tests/eval/), lgtm from @pratik-naik003 since Oct 9, CI green, and it still merges cleanly with main. Happy to rebase or change anything. |
|
/milestone v0.2 |
|
/retest |
|
|
||
| @pytest.mark.parametrize("persona", ["readonly", "data-scientist", "ml-engineer"]) | ||
| async def test_hidden_tools_cannot_be_called(persona, k8s): | ||
| hidden = sorted(set(GATE_ARGS) - get_allowed_tools(persona)) |
There was a problem hiding this comment.
These cases only exercise hidden mutating tools because GATE_ARGS contains only mutators. A regression that allows a persona to call a hidden read-only tool would still pass. Could you include valid arguments for hidden read-only tools and assert they are rejected in full and proxy modes?
The hidden-tool checks only tried tools from GATE_ARGS, which holds mutating tools, so a regression that let a persona call a hidden read-only tool (inspect_crd, inspect_controller, wait_for_training) still passed. READ_ARGS gives valid arguments for every read-only tool that some persona hides, and a guard test fails when a new one is missing. Both hidden-tool checks, full mode and through execute_tool, now call every hidden tool and assert the fake API saw no request at all, reads included. Signed-off-by: Abhishek <abhikokadwar2@gmail.com>
|
Good catch @abhijeet-dhumal, thanks. Done in 36f2885.
To check the gap is closed I broke the persona filter two ways on current main (9424baf):
42 pass on this branch and on current main; ruff clean. |
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…ing tool Replace the delete_training_job-only end-to-end test with one that covers every mutating tool. For each tool it sends 23 confirmed values (the string and numeric forms of true and false, plus "maybe", "", "false ", None and []) through full mode and through execute_tool in progressive and semantic mode, against a fake Kubernetes API that records writes. The outcome (preview, error or success) and the writes must match full mode, so a later change to execute_tool cannot drift from it again. Parity alone would pass if every mode were wrong the same way, so false-like values must also preview and non-boolean values must be rejected, both without a write. test_every_mutating_tool_has_gate_arguments fails until a new mutating tool is added to GATE_ARGS. Against main's dynamic_tools.py the test fails for all 8 tools. GATE_ARGS and the fake API mirror tests/eval/conftest.py from kubeflow#346 so the test can move onto that fixture once it lands. The fake returns {} rather than a MagicMock: a mock ends up in tool responses and FastMCP spends ~0.3s per call failing to serialize it, which made the test take 20s instead of 2s. Signed-off-by: NSTKrishna <krishnagehlot936@gmail.com>
Krishna-kg732
left a comment
Krishna-kg732
left a comment
There was a problem hiding this comment.
Thanks @abhishekKokadwar! I ran these on main with #348 merged and with #484 merged, and all 42 pass in both.
Since the last push already dropped the lgtm, could you fold in @pratik-naik003's two nits here instead of a follow-up? The platform-admin case still returns before asserting anything (line 125), and GATE_ARGS, READ_ARGS and payload are still imported from conftest.py (line 35).
Note: this review was assisted by Claude.
…t of conftest Two follow-ups from review on kubeflow#346. The persona allowlist check returned early for platform-admin, so that case passed without checking anything. It now asserts that an unrestricted persona sees every tool the loaded clients and the server layer register. With one read-only tool hidden from unrestricted personas in create_server, the old check passed and this one fails. GATE_ARGS, READ_ARGS and payload move from conftest.py to tests/eval/helpers.py. pytest loads conftest.py itself, so importing from it can load the module twice. conftest.py keeps only the fixtures. Signed-off-by: Abhishek <abhikokadwar2@gmail.com>
|
Thanks @Krishna-kg732, done in 1cbf8de, so there is no separate follow-up.
42 pass on this branch and with the tests on current main (bba6d67); ruff clean. |
Description
Implements #325. No objection has come up on the issue since Oct 3, so I am marking this ready for review. Happy to adjust or close it if the direction should change.
ROADMAP.md (Phase 2) and ARCHITECTURE.md ("Eval Pipeline") list rule-based safety judges that run on every PR as Tier 1 of the eval pipeline. This adds them as plain pytest under
tests/eval/: 41 checks, about 9 seconds locally, no cluster, no model, no new dependency and no workflow change. They are picked up by the existingmake test-python.What it checks
The per-tool unit tests check each preview one tool at a time. These checks cover the rules that hold across the tools of every registered client, through an in-memory MCP client:
confirmedparameter, andconfirmeddefaults toFalse.confirmed=Falsereturns a preview and sends no POST, PUT, PATCH or DELETE, for every mutating tool. A control checks thatconfirmed=Truedoes write, so this cannot pass trivially.readonlyexposes no mutating tool, and hidden tools cannot be called.execute_toolin progressive and semantic modes.GATE_ARGS, so a new mutating tool fails the suite until it is covered.Kubernetes is faked at
ApiClient.call_api, the one place both the SDK and the raw clients go through.Scope and limits
CLIENT_MODULES, so a new client's tools are checked without editing the test file, and the suite fails until its mutating tools are listed inGATE_ARGS. On main only the trainer client registers tools today, so the count is still 41. A client that talks to its own API server instead of the Kubernetes API would also need a fake for that API.Relation to existing tests
trainer/api/architecture_test.pychecks the persona tables and annotations by calling the policy functions. These check what a running server lists and lets a client call.Related Issue
Fixes #325
Checklist
make test-python)make verify)git commit -s)Testing
Unit tests
Integration tests
E2E tests
Manually tested (describe below)
On main at 4bdc6d0: the 41 new checks pass, and the full suite is 932 passed, 8 skipped.
ruff checkandruff format --checkare clean. I ran these directly on Windows instead of throughmake.To show the checks fail when they should, I broke the server on purpose in 9 ways and ran the existing suite and these checks against each break. This was measured on the earlier base 65582b4, before the rebase:
fine_tunehasconfirmeddefault toTruefine_tuneannotated read-onlycreate_serverSecond commit (63c0c4d), checked against the Spark branch from feat: add spark client module for SparkConnect sessions #51 (4890fd4) with the test files copied in unchanged:
create_spark_sessionanddelete_spark_sessionadded toGATE_ARGS: 47 passdelete_spark_sessionmade to skip the gate: 3 checks fail and name that toolBefore this commit the checks built a trainer-only server, so they did not look at the Spark tools at all. On main the 41 checks still pass and still fail on three of the deliberate breaks above (persona filter bypassed, proxy modes expose every tool, a tool skipping the gate).