Address review findings: tie the query-request path back to the contract

Review finding:

The new rule hardcodes the contract path '/users/456' (and repeats the full request string '/users/456?fields=name&verbose=true' in its message), duplicating a value whose single source of truth is tools/http/test-client/contract.json. Nothing ties the two together: no test reads contract.json, and the equality check fails silently rather than erroring, so editing the query request's path in contract.json would quietly stop the url.query check from ever firing and every affected coverage file would simply lose a finding. A fix is a small test in tools/http/runner/tests that loads tools/http/test-client/contract.json and asserts it still contains a GET request whose path starts with '/users/456?', so the drift fails loudly.

Review finding:

The query request's path changed from '/users/123?...' to '/users/456?...' purely so the HTTP policy can identify that request, but the exchange's own 'description' field still only says the request is about a query string. contract.json documents itself as the description read by every language rather than restated elsewhere, so a reader or future editor of this file has no way to know the '456' is load-bearing and could normalise it back to '/users/123' for consistency, silently disabling the url.query check. A fix is to add a clause to that description saying the path differs from the other /users request so validation can identify this one when url.query is missing.

Analysis: The policy recognises the contract's query request by matching url.path against a literal path, and the finding message repeats the full request target. Rego evaluates a failed equality as "no finding", so a contract edit produces silence rather than an error. The policy test now reads contract.json, takes the single GET request carrying a query, and builds its sample spans from that path and query instead of from literals; it also asserts the finding message quotes the contract's exact request target. Any spelling difference between the contract and the policy now fails test_contract_query_request_requires_url_query rather than passing quietly. The contract entry's own description records that its path is deliberately distinct, which is the field this repository already uses to carry a request's purpose to every language that reads the contract.

Upsides: A contract edit that would disable the url.query check fails a test instead of silently removing a finding from every affected coverage file. The reason the path is distinct is recorded in the file that holds the value, not only in two READMEs a maintainer editing JSON may not open.

Downsides: The policy test now depends on tools/http/test-client/contract.json, so a structural change to the contract file breaks a runner test as well as the test-client tests. The assertion that exactly one GET request carries a query string is an assumption the contract could outgrow, and adding a second query request would require choosing which one the policy identifies anyway.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: trask

tools/http/runner/tests/test_http_policy.py

@@ -16,13 +16,35 @@
 from opentelemetry.conformance import WeaverNotInstalledError, check_weaver
 
 _POLICIES = Path(__file__).parents[1] / "src/http_conformance/policies"
+_CONTRACT = Path(__file__).parents[2] / "test-client/contract.json"
 _HTTP_POLICY_IDS = {
     "http_route_not_present",
     "http_span_name_format",
     "required_attribute_not_present",
 }
 
 
+def _contract_query_request() -> tuple[str, str]:
+    """The contract's one query request, split into its path and its query.
+
+    Read from the contract rather than repeated here: the policy recognises
+    that request by its path, so a path the two sides spell differently would
+    leave the `url.query` check firing on nothing and reporting nothing.
+    """
+    requests = json.loads(_CONTRACT.read_text(encoding="utf-8"))["requests"]
+    targets = [
+        request["path"]
+        for request in requests
+        if request["method"] == "GET" and "?" in request["path"]
+    ]
+    assert len(targets) == 1, f"expected one query request, got {targets}"
+    path, _, query = targets[0].partition("?")
+    return path, query
+
+
+_QUERY_PATH, _QUERY_STRING = _contract_query_request()
+
+
 def _server_span(
     name: str,
     route: str | None = None,
@@ -113,13 +135,13 @@ def policy_advice(
                 _server_span(
                     "GET /users/{query}",
                     route="/users/{query}",
-                    path="/users/456",
+                    path=_QUERY_PATH,
                 ),
                 _server_span(
                     "GET /users/{with_query}",
                     route="/users/{with_query}",
-                    path="/users/456",
-                    query="fields=name&verbose=true",
+                    path=_QUERY_PATH,
+                    query=_QUERY_STRING,
                 ),
             ]
         ),
@@ -245,6 +267,10 @@ def test_contract_query_request_requires_url_query(
         "attribute_key": "url.query",
         "kind": "server",
     }
+    assert (
+        f"'{_QUERY_PATH}?{_QUERY_STRING}'"
+        in advice["required_attribute_not_present"]["message"]
+    )
 
 
 def test_contract_query_request_with_url_query_has_no_finding(

tools/http/test-client/contract.json

@@ -17,7 +17,7 @@
       "responseBody": "{\"id\": 123, \"name\": \"Alice\"}"
     },
     {
-      "description": "A query string, which is its own attribute (`url.query`) and must not leak into `http.route`, `url.path` or the span name.",
+      "description": "A query string, which is its own attribute (`url.query`) and must not leak into `http.route`, `url.path` or the span name. Its path differs from the other `/users` request so that validation can identify this one when `url.query` is missing.",
       "method": "GET",
       "path": "/users/456?fields=name&verbose=true",
       "status": 200,