[Atom-oh]

Stacked on #183. Merge #183 first, then retarget this PR to dev.

This adds agent/lambda/test_external_connections.py (544 lines, 121 tests) with no changes. It was split out of #183 because that PR's diff (131 KiB) went over the 128 KiB AI review input limit, so the review never reached a model.

The tests cover connection identity binding, bounded reads, shared request deadlines, query-proof verification and result-completion disclosure for the datasource connector Lambdas. docs/runbooks/datasource-credential-isolation.md in #183 already points to this file.

Verification: cd agent/lambda && python3 -m pytest test_external_connections.py -q → 121 passed.

🤖 Generated with Claude Code

[github-actions]

🤖 AI Code Review (two independent reviewers)

_Reviewer responses: codex/ALL claude/ALL _

Status: PASSED — No blocking issues found

Spot verification confirms the panel claims: all referenced base symbols exist (MAX_SERIES, set_request_conn, _ApiError, _SCHEMA_NAMES_TIMEOUT_S, skippedCount, clickhouse_health with accepted=("ok","partial")), the runbook at docs/runbooks/datasource-credential-isolation.md:68 already lists test_dynatrace_mcp.py (new in this PR) but not test_external_connections.py, and scripts/v2/merge-verify.sh:23 auto-discovers test_*.py. No CRITICAL/MAJOR from either panel survived or emerged. Final synthesized review follows.

---

PR #191 — Chair Review: test(datasources): connection identity and bounded I/O regressions

1. Summary

This PR is test-only: it adds agent/lambda/test_dynatrace_mcp.py and agent/lambda/test_external_connections.py, pinning the datasource connectors' connection-identity isolation, bounded HTTP I/O (byte caps, shared request deadlines, error-body non-echo), query-proof health probes, and result-completion (collectionStatus/truncated/limitSaturated) contracts. Both panel models independently found no CRITICAL or MAJOR issues; chair spot-verification against BASE confirms every symbol the tests import exists (datasource_http, dynatrace_mcp.MAX_SERIES, loki_mcp._query_response, jaeger_mcp.skippedCount, tempo_mcp._SCHEMA_NAMES_TIMEOUT_S, clickhouse_mcp.clickhouse_health) and that CI auto-discovers the new files via scripts/v2/merge-verify.sh. The findings below are all MINOR hardening/documentation suggestions.

2. Issues per lens

L2 — Code correctness

No CRITICAL/MAJOR. Codex found nothing (its review was static due to an ENOSPC sandbox failure); Claude traced the non-obvious expectations (Mimir prefix de-dup, timeout ceiling table, Jaeger unpadded-hex ID acceptance, ObservedPayload.http_status attribute precedence) and they match the implementations. Chair confirmed the referenced symbols and the test_*.py auto-discovery in CI.

• MINOR (one model) — Tight wall-clock margins: test_header_phase_is_bounded_by_the_request_deadline (elapsed < 0.5 for a 0.2 s deadline) and test_blocking_socket_read_uses_only_budget_remaining_after_headers (< 2.0 including a 1.0 s sleep) may flake on loaded shared runners.
• MINOR (one model) — test_header_deadline_closes_the_tracked_connection can fail as a bare KeyError on held["conn"] if the pool worker never starts within the budget (the shared _OPEN_POOL has 4 workers and neighbouring tests leave workers sleeping). Passes in definition order today, but the failure mode is illegible.
• MINOR (one model, chair-verified) — The tempo discovery-budget assertion accepts _SCHEMA_NAMES_TIMEOUT_S only by coincidence: allowed = {dh.HTTP_TIMEOUT, _SCHEMA_TIMEOUT_S} and _SCHEMA_NAMES_TIMEOUT_S happens to equal HTTP_TIMEOUT (12). Chair confirmed both constants at tempo_mcp.py:33-34. Changing HTTP_TIMEOUT alone would fail the test on intended behaviour.
• MINOR (one model) — test_unrepresentable_chart_numbers_are_unknown_without_serializing_them asserts only collectionStatus == "unknown"; it never checks the raw body for Infinity/NaN, so the "without serializing them" half of the name is unverified (json.dumps with allow_nan on would emit bare Infinity and round-trip silently).
• MINOR (one model) — patch.object(dh.time, "monotonic", …) patches the stdlib time module process-wide; it works only because concurrent waits are C-level. Also two readability nits: a local trace dict shadows the module-level trace() helper, and the Loki case in the transport-evidence spoof test doesn't actually carry an embedded http_status key, so that half of the title is exercised for the other three connectors only.

L3 — Security / AWS mutation safety

No findings — both models agree. No IAM, Terraform, or production connector code changes; the ADR-005 frozen-mutation boundary is untouched (all exercised connectors remain readers, ClickHouse behind readonly=1 + the read-only SQL guard). All credential-shaped literals are obvious synthetic fixtures against the reserved-TLD host provider.example.test. The patch strengthens security coverage: SSRF classification on the probe path for all eight connectors (with http_json stubbed to AssertionError so a pre-guard request would fail the test), bounded/closed/never-echoed error bodies, authType: "none" suppressing legacy Datadog keys, and explicit-instance credentials never borrowing the kind mirror. One model noted as a non-blocking follow-up (not a regression here): the direct query tools still fold up to 300 chars of upstream error text into _ApiError → err(str(e)), which a header-reflecting provider could abuse — worth a ticket, not a change in this PR.

L4 — Observability / data-integration correctness

No CRITICAL/MAJOR — both models agree the completion-contract coverage matches the producing code (Dynatrace caps/pagination/warning demotion, Loki limitSaturated vs truncated split, Jaeger saturation-vs-loss and zero-span disclosure, http_status-spoof resistance). No Steampipe, CIS, or AgentCore diagnosis logic is touched.

• MINOR (one model, coverage gap) — The Loki envelope-level source_unverified demotion (non-list warnings, non-bool partial/truncated at loki_mcp.py:85-90) is not exercised; a {"warnings": 5} case would close the last unguarded branch of that classifier.
• Observation worth a code comment: the ClickHouse probe can never reach "ok" (its 1-row probe always saturates → "partial"), so the accepted=("ok", "partial") tuple at clickhouse_mcp.py:284 is load-bearing — the new test pins this, but a comment would prevent someone later "tightening" accepted and breaking the Test button.

L5 — Docs/ADR consistency

No CRITICAL/MAJOR. ADRs live in the private upstream repo (per root CLAUDE.md), so no ADR text exists here to reconcile; ADR-005 is satisfied on the merits. Notably, the PR repairs a docs mismatch: docs/runbooks/datasource-credential-isolation.md:68 already instructs running test_dynatrace_mcp.py, which did not exist until this PR (chair-verified).

• MINOR (one model, chair-verified) — That same runbook command does not list test_external_connections.py, yet that file is where the credential-isolation regressions it describes (explicit instance never borrowing the kind mirror; legacy default not lending to siblings; authType: none suppressing stored Datadog keys) actually live, while the listed test_dynatrace_mcp.py contains no isolation case. Add the new file to the line-68 command.
• MINOR (one model) — The connection-test contract ({ok, verified, latency_ms, error?}, query-proof-not-/ready, fixed non-echoing error sentences) is now pinned by tests but documented nowhere under docs/; a short paragraph in the credential-isolation runbook or observability reference would keep the operator-visible contract out of test-file-only lore.

3. Suggestions

1. Loosen the two wall-clock assertions (or mark them with a generous multiplier) to reduce CI flake risk.
2. Guard held["conn"] with held.get("conn") plus an explicit assertion/skip so a pool-scheduling miss fails legibly.
3. Add getattr(module, "_SCHEMA_NAMES_TIMEOUT_S", dh.HTTP_TIMEOUT) to the tempo allowed set.
4. Assert "Infinity" not in body and "NaN" not in body on the raw body in the unrepresentable-numbers test.
5. Add a Loki {"warnings": 5} → unknown case, and a one-line comment on the ClickHouse accepted=("ok", "partial") pin.
6. Update docs/runbooks/datasource-credential-isolation.md:68 to include test_external_connections.py, and document the Test-button probe contract.

4. Verdict

Test-only PR; no production logic, IAM, or infrastructure change. All findings are MINOR and non-blocking. Both panels and the chair declared image coverage not required (the manifest lists zero images, zero deletions, zero omissions, status complete).

IMAGE_COVERAGE: NOT_REQUIRED

---

Triggered by commit 50ffa25c80d6dbc83f06fb61a1e824ec8c13675b · workflow: .github/workflows/pr-review.yml