test(trace-adapter): exercise every refusal finalize_trace can reach - #31
Conversation
`trace_adapter.py` has sixteen `raise TraceFinalizationError` sites. Replacing each one with `pass` on its own and re-running the suite leaves ten of them green, so ten correct refusals had nothing exercising them. With this file the same sweep turns all sixteen red. The sites are located by AST rather than by grep, so the two compound conditions and the two `except` handlers are counted alongside the twelve plain guards. The eight cases that a bad event stream can cause are built through `EvidenceAccumulator` rather than by handing the adapter a dict, so a passing case is also evidence the refusal is reachable from a schema-valid event stream rather than defensive. That distinction matters for at least one of them: `policy.required` is `["engine", "engine_version"]`, so a conforming producer can emit a decision without a `bundle_digest`, and the guard is the only thing between that and a signed record. Two of the sixteen are different in kind and the file says so where they sit. No event stream can produce either. The missing optional dependency is reached by blocking the import, which is the only way in, so that case does not go through `finalize_trace` at all. The signing wrapper does go through `finalize_trace`, on an accumulator-built snapshot, but it needs a key that satisfies the shape check and then fails when used, because `object()` is refused earlier for having no `sign` and a case built that way would pass on the wrong refusal. Behaviour is unchanged. Probing `_policy_binding` directly with six malformed event streams produced the correct refusal every time before this was written; the gap was coverage, not conduct. CI as configured, in order: check_versions, check_schemas, check_otel_compatibility, check_typescript_schemas, validate.py, unittest discover, compileall, and on the 3.12 leg build and smoke_wheel on both artefacts. All exit 0. sync_schemas is not a CI step, it is on the pull request template's evidence list; it also exits 0 and leaves the tree unchanged. On 3.11, 3.12 and 3.13 the suite goes from `Ran 98 tests` to `Ran 108 tests`. On 3.10 it stays at `Ran 89 tests` and gains one skip: `agentrust-trace` is pinned to `python_version >= '3.11'`, so this class skips whole on the same guard and the same message as `tests/test_trace_adapter.py`. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>
|
❔ Contributor Check: UNKNOWN
Automated check by AgenTrust Contributor Check. |
imran-siddique
left a comment
There was a problem hiding this comment.
Verified the number this rests on rather than taking it. Parsed src/agentrust_telemetry/trace_adapter.py at pinned 0737e548 with ast: 16 raise TraceFinalizationError sites, at lines 96, 106, 108, 110, 116, 127, 129, 135, 137, 143, 151, 154, 164, 169, 174 and 260. Sixteen, none excluded, exactly as you say.
Counting by AST rather than by grep is the part that makes that number trustworthy. A text match would have missed or miscounted the two compound conditions and the two except handlers, and those four are precisely the ones a hand-written test suite tends to leave uncovered, because they are harder to reach than the twelve plain guards.
Reading the exit code rather than a neighbouring runner's output is the other half. A mutation harness that greps a test runner's summary text is measuring the harness, not the suite, and it fails silently when the output format changes. python -m unittest discover -s tests under the same invocation CI uses, judged by exit status, measures the thing that actually gates the merge.
Ten of sixteen returning exit 0 with their guard removed is a real finding about this file. Those ten guards existed and nothing would have noticed their removal, which is the same as not having them once someone refactors.
Two things I want to credit beyond the method.
Tracing each case to the line it trips rather than inferring from the error message. Two guards raising similar text is common, and message-matching produces a suite that looks disjoint and is not.
Checking disjointness in both directions against tests/test_trace_adapter.py. Adding ten cases that duplicate existing coverage would still have shown sixteen-of-sixteen afterwards while adding nothing, and only the reverse check rules that out.
No change under src/, all eight checks green. Approving and merging.
Separately and not for you to action: this repo was not on my triage list and its PRs have been going unseen. Fixing that at my end.
Summary
One test file. Ten cases and six subtests covering every refusal
finalize_tracecan reach, and no change undersrc/.The reason for it:
trace_adapter.pyhas sixteenraise TraceFinalizationErrorsites, and removing each one on its own leaves ten of them with nothing to notice.Measured by
python -m unittest discover -s tests, which is what CI runs, by exit code rather than by a neighbouring runner's output. Neutralised means theraisestatement replaced bypass, located by AST rather than by grep, so the two compound conditions and the twoexcepthandlers are counted alongside the twelve plain guards. Sixteen statements, none excluded.The ten were: non-empty chained evidence, the six required configuration fields, duplicate
classification_order, no policy decision present,bundle_digestabsent, conflicting enforcement modes, no data-flow evidence, taxonomy conflict, the missing optional dependency, and the signing wrapper.Each of the ten cases trips exactly one of the sixteen sites, traced at the line rather than inferred from the message, and the ten are disjoint from the six that
tests/test_trace_adapter.pyalready holds up. Checked the other way round as well: for all ten, the string the test asserts on is a literal inside theraiseat the site the tracer saw, read out of both files by AST rather than by eye. The one exception ishardware signer unavailable, which is the nested cause interpolated at runtime rather than a literal, and that is the point of that case. Nothing here duplicates a case you have, and no case passes on a refusal other than the one it names.setUpClassis copied fromtests/test_trace_adapter.pyunchanged, including the Python 3.11 guard and its message, so the two files skip and configure identically.Behaviour is unchanged and was already correct. Probing
_policy_bindingdirectly with six malformed event streams produced the right refusal every time before any of this was written. The gap was coverage, not conduct.Two things the cases are built to show:
They are reachable rather than defensive. The eight cases that a bad event stream can cause go through
EvidenceAccumulator, not by handing the adapter a dict, so a passing case is also evidence the refusal can be triggered from a schema-valid event stream. That matters most forbundle_digest:policy.requiredis["engine", "engine_version"], so a conforming producer can emit a policy decision without a digest, and the guard is the only thing between that and a signed record. The case assertsself.validator.validate(event)first, so the file records that the schema permits what the adapter refuses.Two of the sixteen are different in kind, and the file says so where they sit. No event stream can produce either. The missing optional dependency is reached by blocking the import, which is the only way in, so that case does not go through
finalize_traceat all. The signing wrapper does go throughfinalize_trace, on an accumulator-built snapshot, but it needs a key that satisfies the shape check and then fails when used:object()is refused earlier for having nosign, so a case built that way passes on the wrong refusal. That was the first version of this test and it is called out in the file so nobody rebuilds it.Contract impact
No schema, fixture, or
src/file is touched. The diff is one added file undertests/.Evidence
python tools/sync_schemas.py(exit 0, working tree unchanged after)python tools/check_schemas.pypython conformance/runner/validate.pypython -m unittest discover -s tests -vpython -m buildAlso run, since CI runs them:
check_versions.py,check_otel_compatibility.py,check_typescript_schemas.py,compileall -q src, andtools/smoke_wheel.pyon both the wheel and the sdist. All exit 0.Not only on my machine.
ci.ymlunmodified, dispatched on GitHub's runners against the branch and againstmain, on the fork and before opening this, so the checks on this pull request are the authoritative ones and these are only what I had before asking you to look. Ten jobs across the two runs, every jobsuccessand no step failed. On 3.10, 3.11 and 3.13 thebuildandsmoke_wheelsteps reportskipped, which is the workflow's ownif: matrix.python-version == '3.12'doing its job rather than a step that did not run:test/trace-adapter-refusalsat7d81258: https://github.com/lywinged/agentrust-telemetry/actions/runs/32792667475mainat0737e54, for comparison: https://github.com/lywinged/agentrust-telemetry/actions/runs/32789449660That branch run's own log, on 3.11, prints
Ran 108 tests in 2.810sandOK (skipped=2).That log is where the 3.11 row below comes from. The other three rows are from fresh clones here; on CI those legs are confirmed by exit code rather than by a line I quoted.
Ran 89 tests, 4 skippedRan 89 tests, 5 skippedRan 98 tests, 2 skippedRan 108 tests, 2 skippedRan 98 tests, 2 skippedRan 108 tests, 2 skippedRan 98 tests, 2 skippedRan 108 tests, 2 skippedExit 0 in every cell. The two standing skips are
PyYAML not installedand are onmaintoo.Also measured against the open #30, since it touches the same two files. On that tree the same ten sites survive being neutralised and the same six are caught, so nothing here overlaps it. The two merge in either order without a conflict, and the merged tree is
Ran 108 testsat exit 0 with all sixteen sites red under the same sweep.Privacy and security
Every identifier in the file is under
example.testor anexample.*name, and the configuration block is copied verbatim fromtests/test_trace_adapter.py. The run idrun-governed-sdlc-001and the event ids are the synthetic ones already inconformance/fixtures/valid/, varied in the last byte the same way your own adapter tests vary them. No new fields, so the second box does not apply. The third is yours to judge: nothing undersrc/changes, but one case constructs a signing key that fails when used, so it is signing-adjacent by subject even though it cannot alter behaviour.Limits
tests/test_trace_adapter.py, becauseagentrust-traceis pinned topython_version >= '3.11'. One of the four matrix legs gains no coverage from this.validation.py,context.py, or the TypeScript package. The sweep coversraise TraceFinalizationErrorintrace_adapter.pyand no other module.One question rather than a change
classification_taxonomyis not in therequireddict that_require_finalizablechecks, so an empty one is caught later bydata-flow taxonomy conflicts with TRACE configuration. It is refused either way. The diagnosis is the less accurate of the two, and I did not want to pin current behaviour in a test without asking whether the omission is deliberate.