Repository navigation
conformance(runner): reject boolean/float level, invalid fmt, and malformed caller options at run() entry (#626) - #631
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
rajnisht7
left a comment
There was a problem hiding this comment.
fmt validation is not fully robust. Because _VALID_FORMATS is a frozenset, unhashable values like [] or {} raise TypeError: unhashable type instead of the intended ValueError.
kindly change it to:
if not isinstance(fmt, str) or fmt not in _VALID_FORMATS:
raise ValueError(...)and add [] / {} to the bad_fmt tests for both run() and _run_levels().
Also, the optional-arg tests currently only check expected_nonce / receipt / entry on run(), kindly cover them on _run_levels() too, since the PR claims validation at both entry point
lywinged
left a comment
There was a problem hiding this comment.
Reviewed at 4efc8cd on da69b31. Not approving yet: both points in rajnisht7's review hold here, and I would wait for both. run({}, [], level=0) raises TypeError: unhashable type: 'list'. And passing None for expected_nonce, receipt and entry in the _validate_runner_inputs() call inside _run_levels() leaves every test passing, so that call has no test that fails when it is switched off, which CONTRIBUTING's "Before a pull request is reviewed" asks for. I will review the rest once both are in.
CI at 4efc8cd failed in the portability step for a reason this pull request did not cause: main failed the same step at da69b31, before #624, and passes at dcca4e0.
Generated by Claude Code
…alid options at run() entry (agentrust-io#626) Require type(level) is int, isinstance(record, dict), isinstance(fmt, str) and fmt in _VALID_FORMATS, positive integer max_age_seconds, and valid optional expected_nonce, receipt, entry, and policy_resolver at the entry of both run() and _run_levels() in conformance/src/trace_tests/runner.py. Closes agentrust-io#626 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
|
Thank you @rajnisht7 and @lywinged! Addressed both points in
|
4efc8cd to
d8e5379
Compare
lywinged
left a comment
There was a problem hiding this comment.
Approve. Reviewed at d8e5379 on e9ab302.
Both points from the first round are in. run({}, [], level=0) and run({}, {}, level=0) raise ValueError here, where 4efc8cd raised TypeError, and the _validate_runner_inputs() call in _run_levels() now has tests behind it: removing it fails the same 19 tests in the new tests/ file as removing the call in run(), since each of those tests asserts run() first and _run_levels() second.
What I checked rather than read:
- CI's commands, run here on 3.11 and 3.12: ruff and mypy as CI runs them, which do not reach
conformance/, and the dash check clean; the suite at 5245 passed and 98 skipped on both, with the head's conformance package on the path as CI installs it; the replay experiment, the twoLC_ALL=C PYTHONUTF8=0runs, thePYTHONOPTIMIZE=2command and the two--helpcalls pass. The second of those two runs is the conformance suite, 983 passed, 2 skipped and 5 xpassed on both; 3.13, which Conformance CI also runs, is not installed here. - Every one of the 69 JSON files under
conformance/tests/vectors, given torun()as a"trace"record at levels 0, 1 and 2 with default options, gives the same findings onmainand here; 13 of them are records and 56 are vector wrappers that fail the same way on both. - Removed alone from the head,
isinstance(fmt, str)fails 2 tests in the newtests/file, the level type check 3, and either_validate_runner_inputs()call 19; the conformance copy of the same tests fails as many again. tools/review_bar.pyreports 19 of the 25 cases in the newtests/file failing on behaviour at the merge base and 3 passing there, the"0",Noneand-1levels; the 3 it sorts as a missing name are theAttributeErrorof the non-dict records, a sorting the tool's own note warns about. The description's line still says 17.
Things I would not hold the merge for:
run(record, "cmcp-runtime", level=0)raisesKeyError: 'trace'for{}and for{"cmcp_version": "1.0"}, andAttributeErrorfor{"trace": None}, as onmain:extract_trace()readsrecord["trace"]for that format, and the record check at the entry point only looks at the outer object. These two lines after the format check turn all three intoValueError, and the conformance suite still passes:
if fmt == "cmcp-runtime" and not isinstance(record.get("trace"), dict):
raise ValueError("a cmcp-runtime record must carry a 'trace' object")- Two lines of the description. Its "Spec section" names section 5 of
spec/trace-v0.2.md, which is the cMCP reference implementation; the levels are described inconformance/docs/levels.md, and the spec's one mention of the suite is in section 3.2.2. The pytest command under validation stops withImportPathMismatchErrorhere, becausetests/andconformance/tests/are both packages namedtests; run separately, the three files pass.
Generated by Claude Code
What this changes
Closes #626.
conformance/src/trace_tests/runner.py(run,_run_levels):type(level) is int and level in _LEVEL_MODULESinrun()(matching_run_levels()), rejecting booleans (True/False) and floats (1.0).isinstance(record, dict),fmt in ("trace", "cmcp-runtime"),type(max_age_seconds) is int and max_age_seconds >= 1,expected_nonce is None or isinstance(expected_nonce, str),receipt is None or isinstance(receipt, dict),entry is None or isinstance(entry, dict),policy_resolver is None or callable(policy_resolver)) at entry in bothrun()and_run_levels().Type of change
Spec section
spec/trace-v0.2.mdsection 5 (Conformance Levels).Tests for a fix
tests.test_conformance_runner_option_boundaries::test_run_rejects_boolean_and_non_int_leveltests.test_conformance_runner_option_boundaries::test_runner_rejects_non_dict_recordtests.test_conformance_runner_option_boundaries::test_runner_rejects_unknown_formattests.test_conformance_runner_option_boundaries::test_runner_rejects_invalid_max_age_secondstests.test_conformance_runner_option_boundaries::test_runner_rejects_invalid_optional_argumentspython -m pytest conformance/tests/unit/test_runner.py conformance/tests/test_obligation_accounting.py tests/test_conformance_runner_option_boundaries.py -q-> passedpython tools/review_bar.py --base origin/main --head HEAD->17 fail on behaviourChangelog entry
trace_tests.runner): require exactintlevelinrun()and validaterecord,fmt,max_age_seconds,expected_nonce,receipt,entry, andpolicy_resolveratrun()and_run_levels()entry (conformance(runner): reject boolean/float level, invalid fmt, and malformed caller options at run() entry #626, @drk-ram).Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text