Skip to content

Commit 45747c8

Browse files
cyntwang99claude
andcommitted
fix(tracing): keep the commit SHA out of the shared business span
trace.py hands ONE Span instance to every registered processor, and _add_source_to_span mutates span.data in place. So writing __commit_sha__ there leaked it: a co-registered Agentex processor serialized it too, and it surfaced in caller-visible span.data -- contradicting the claim that this field is SGP-scoped. Build the SGP write's metadata as a copy instead. Adds a regression test that asserts the SGP metadata carries the key while the shared span and the Agentex processor's payload do not, plus one for list-shaped data, which has nowhere to put a metadata key and is now returned untouched. The __source__ / __agent_* keys leak the same way today; left alone deliberately, since changing five long-shipped fields is out of scope here. Reported in review by Greptile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 4d860fc commit 45747c8

2 files changed

Lines changed: 69 additions & 22 deletions

File tree

src/agentex/lib/core/tracing/processors/sgp_tracing_processor.py

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import os
44
import asyncio
55
import weakref
6-
from typing import cast, override
6+
from typing import Any, cast, override
77

88
import scale_gp_beta.lib.tracing as tracing
99
from scale_gp_beta import SGPClient, AsyncSGPClient
@@ -68,11 +68,29 @@ def _add_source_to_span(span: Span, env_vars: EnvironmentVariables) -> None:
6868
span.data["__agent_id__"] = env_vars.AGENT_ID
6969
if env_vars.AGENT_VERSION is not None:
7070
span.data["__agent_version__"] = env_vars.AGENT_VERSION
71-
# Opt-in only (adk.code_revision.enable()); None unless the agent asked
72-
# for it, so no agent inherits this by upgrading the SDK.
73-
commit_sha = code_revision.commit_sha()
74-
if commit_sha is not None:
75-
span.data[code_revision.COMMIT_SHA_KEY] = commit_sha
71+
72+
73+
def _sgp_metadata(span: Span) -> Any:
74+
"""Metadata for the SGP write: ``span.data`` plus the opt-in commit SHA.
75+
76+
Returns a COPY rather than mutating ``span``. ``trace.py`` hands the same
77+
Span instance to every registered processor, so anything written onto
78+
``span.data`` here would also be serialized by the Agentex processor and
79+
show up in caller-visible span data. ``__commit_sha__`` is opt-in and
80+
SGP-scoped, so it must not leak that way.
81+
82+
(The ``__source__`` / ``__agent_*`` keys set by ``_add_source_to_span`` do
83+
leak like that today. Left as-is: changing five long-shipped fields is not
84+
this change's business.)
85+
"""
86+
commit_sha = code_revision.commit_sha()
87+
if commit_sha is None:
88+
return span.data
89+
if isinstance(span.data, dict):
90+
return {**span.data, code_revision.COMMIT_SHA_KEY: commit_sha}
91+
# List-shaped data is an accepted `data` shape and has nowhere to put a
92+
# metadata key; leave it untouched rather than dropping the caller's data.
93+
return span.data
7694

7795

7896
def _build_sgp_span(span: Span, env_vars: EnvironmentVariables) -> SGPSpan:
@@ -88,7 +106,7 @@ def _build_sgp_span(span: Span, env_vars: EnvironmentVariables) -> SGPSpan:
88106
trace_id=span.trace_id,
89107
input=span.input,
90108
output=span.output,
91-
metadata=span.data,
109+
metadata=_sgp_metadata(span),
92110
),
93111
)
94112
sgp_span.start_time = span.start_time.isoformat() # type: ignore[union-attr]

tests/lib/core/tracing/processors/test_sgp_tracing_processor.py

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -54,34 +54,63 @@ def test_agent_identity_and_version_stamped_into_span_data(self):
5454
"__agent_version__": "sha-abc123",
5555
}
5656

57+
SHA = "b362b171a9c4e1f09d8e7a6b5c4d3e2f1a0b9c8d"
58+
5759
def test_commit_sha_is_not_stamped_without_opt_in(self, monkeypatch):
5860
"""Upgrading the SDK must not start emitting __commit_sha__ on its own,
5961
even when the environment carries a perfectly good SHA."""
6062
from agentex.lib.core.tracing import code_revision
61-
from agentex.lib.core.tracing.processors.sgp_tracing_processor import _add_source_to_span
63+
from agentex.lib.core.tracing.processors.sgp_tracing_processor import _sgp_metadata
6264

63-
monkeypatch.setenv("AGENT_COMMIT_SHA", "b362b171a9c4e1f09d8e7a6b5c4d3e2f1a0b9c8d")
65+
monkeypatch.setenv("AGENT_COMMIT_SHA", self.SHA)
6466
code_revision.disable()
6567

66-
env = MagicMock(ACP_TYPE=None, AGENT_NAME=None, AGENT_ID=None, AGENT_VERSION=None)
67-
span = _make_span()
68-
_add_source_to_span(span, env)
69-
assert isinstance(span.data, dict)
70-
assert "__commit_sha__" not in span.data
68+
span = _make_span(); span.data = {}
69+
assert "__commit_sha__" not in (_sgp_metadata(span) or {})
7170

7271
def test_commit_sha_is_stamped_after_opt_in(self, monkeypatch):
7372
from agentex.lib.core.tracing import code_revision
74-
from agentex.lib.core.tracing.processors.sgp_tracing_processor import _add_source_to_span
73+
from agentex.lib.core.tracing.processors.sgp_tracing_processor import _sgp_metadata
7574

76-
sha = "b362b171a9c4e1f09d8e7a6b5c4d3e2f1a0b9c8d"
77-
monkeypatch.setenv("AGENT_COMMIT_SHA", sha)
75+
monkeypatch.setenv("AGENT_COMMIT_SHA", self.SHA)
7876
code_revision.enable()
7977
try:
80-
env = MagicMock(ACP_TYPE=None, AGENT_NAME=None, AGENT_ID=None, AGENT_VERSION=None)
81-
span = _make_span()
82-
_add_source_to_span(span, env)
83-
assert isinstance(span.data, dict)
84-
assert span.data["__commit_sha__"] == sha
78+
span = _make_span(); span.data = {"caller": "kept"}
79+
metadata = _sgp_metadata(span)
80+
assert metadata["__commit_sha__"] == self.SHA
81+
assert metadata["caller"] == "kept"
82+
finally:
83+
code_revision.disable()
84+
85+
def test_commit_sha_does_not_leak_onto_the_shared_span(self, monkeypatch):
86+
"""trace.py hands ONE Span to every processor. If the commit SHA were
87+
written onto span.data, a co-registered Agentex processor would
88+
serialize it too, and it would surface in caller-visible span data."""
89+
from agentex.lib.core.tracing import code_revision
90+
from agentex.lib.core.tracing.processors.sgp_tracing_processor import _sgp_metadata
91+
from agentex.lib.core.tracing.processors.agentex_tracing_processor import _create_kwargs
92+
93+
monkeypatch.setenv("AGENT_COMMIT_SHA", self.SHA)
94+
code_revision.enable()
95+
try:
96+
span = _make_span(); span.data = {}
97+
assert _sgp_metadata(span)["__commit_sha__"] == self.SHA # SGP sees it
98+
assert "__commit_sha__" not in span.data # the span does not
99+
assert "__commit_sha__" not in (_create_kwargs(span)["data"] or {})
100+
finally:
101+
code_revision.disable()
102+
103+
def test_list_shaped_data_is_left_alone(self, monkeypatch):
104+
"""`data` may be a list of dicts; there is nowhere to put a metadata key,
105+
and dropping the caller's data would be worse than omitting the field."""
106+
from agentex.lib.core.tracing import code_revision
107+
from agentex.lib.core.tracing.processors.sgp_tracing_processor import _sgp_metadata
108+
109+
monkeypatch.setenv("AGENT_COMMIT_SHA", self.SHA)
110+
code_revision.enable()
111+
try:
112+
span = _make_span(); span.data = [{"a": 1}]
113+
assert _sgp_metadata(span) == [{"a": 1}]
85114
finally:
86115
code_revision.disable()
87116

0 commit comments

Comments
 (0)