test: harden ingest gates and sharpen SDK copy#316
Conversation
There was a problem hiding this comment.
Pull request overview
This PR strengthens the SDK’s hosted-ingest regression gates to better match the real dashboard/SDK contract, aligns the production smoke workflow with /api/v1/traces behavior, and sharpens SDK/MCP distribution copy across packaging metadata.
Changes:
- Hardened the local ingest harness and added/updated integration tests to reject hosted-incompatible records (e.g., watermark/meta, missing
type, kind/type mismatch) and to verify traces by exacttrace_id. - Updated the manual dashboard smoke script to write a probe trace and verify that exact trace appears in
/api/v1/traces. - Tightened SDK + MCP copy and added stronger publish-time validation (lint/bandit/full pytest + coverage gate) before PyPI release.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/tests/test_sdk_preflight.py | Extends preflight-plan tests to ensure hosted-ingest regression suites run on key changes. |
| sdk/tests/test_integration_dashboard_script.py | Adds unit tests for helper functions used by the dashboard smoke script. |
| sdk/tests/test_integration_cost_guardrail.py | Strengthens integration assertions and verifies a specific trace via /api/v1/traces?trace_id=.... |
| sdk/tests/test_hosted_ingest_contract.py | Adds explicit hosted-ingest contract regression tests (watermark/meta rejection, HttpSink normalization). |
| sdk/tests/test_e2e_pipeline.py | Updates E2E assertions to enforce hosted-ingest expectations (type==kind, no watermark, no rejected requests). |
| sdk/tests/integration_dashboard.py | Updates production smoke to write a probe trace and verify it by service/trace_id. |
| sdk/tests/conftest.py | Makes the mock ingest server mirror hosted contract more closely (validation + request-level logging). |
| scripts/sdk_preflight.py | Updates targeted test mapping and changes how test targets are selected. |
| .github/workflows/publish.yml | Adds release gates (ruff/bandit/full pytest + coverage) before building/publishing. |
| sdk/pyproject.toml | Updates PyPI description/keywords to match refreshed distribution copy. |
| sdk/PYPI_README.md | Refreshes SDK/MCP positioning and wording. |
| README.md | Mirrors the same copy updates as the PyPI README. |
| CHANGELOG.md | Documents the new gating/copy changes under Unreleased. |
| ops/04-DEFINITION_OF_DONE.md | Adds a required “probe trace_id appears in hosted /api/v1/traces” release proof step. |
| mcp-server/server.json | Updates MCP server description to explicitly “read-only”. |
| mcp-server/README.md | Updates MCP server positioning and wording. |
| mcp-server/package.json | Updates npm description and keywords to match refreshed positioning. |
| docs/incidents/2026-04-03-hosted-ingest-regression.md | Adds RCA documenting the regression and the new preventive gates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _build_traces_url( | ||
| base_url: str, | ||
| *, | ||
| trace_id: Optional[str] = None, | ||
| service: Optional[str] = None, | ||
| ) -> str: | ||
| url = f"{base_url}/api/v1/traces" | ||
| params = [] | ||
| if trace_id: | ||
| params.append(f"trace_id={trace_id}") | ||
| if service: | ||
| params.append(f"service={service}") | ||
| if params: | ||
| return f"{url}?{'&'.join(params)}" | ||
| return url |
There was a problem hiding this comment.
_build_traces_url builds query strings via string concatenation and doesn't URL-encode parameter values or normalize trailing slashes on base_url. If AGENTGUARD_DASHBOARD_URL ends with '/', or if trace_id/service contain reserved characters, this can generate malformed URLs (e.g. double slashes or unescaped query params). Consider using urllib.parse.urljoin + urllib.parse.urlencode (or at least stripping a trailing '/').
| sink.shutdown() | ||
| time.sleep(0.2) | ||
|
|
There was a problem hiding this comment.
HttpSink.shutdown() already flushes synchronously and joins the background thread. The extra time.sleep(0.2) after shutdown adds test latency and can still be flaky on slow CI if it’s masking a real synchronization issue. Prefer asserting on request_log/events immediately after shutdown (or, if needed, add a deterministic wait-for-condition helper rather than a fixed sleep).
| pytest_targets: Set[str] = set() | ||
| for path in normalized: | ||
| if path.startswith("sdk/tests/") and path.endswith(".py") and (REPO_ROOT / path).exists(): | ||
| if ( | ||
| path.startswith("sdk/tests/") | ||
| and path.endswith(".py") | ||
| and Path(path).name.startswith("test_") | ||
| and (REPO_ROOT / path).exists() | ||
| ): |
There was a problem hiding this comment.
build_plan now only auto-adds changed sdk/tests/.py files to pytest_targets when the filename starts with test_. This means edits to non-test scripts under sdk/tests/ (e.g. e2e_cost_guardrail.py, e2e_v110.py) will no longer trigger any targeted pytest run unless explicitly listed in TEST_MAP, potentially letting syntax/import errors slip past preflight. Consider either keeping the previous behavior for explicitly-changed sdk/tests/.py paths, or adding those scripts to TEST_MAP / adding a lightweight import/compile check for non-test modules.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
/api/v1/tracesresponse shapeValidation
Proof
aa20b00d624345c6b5ee59ef78370663under serviceintegration-dashboard-test-1775267564/api/v1/traceswithevent_count=3anderror_count=0