[Atom-oh]

Summary

Part 1 of 3 (stack base). Merge order: this PR → #184 → #185. #184 and #185 are stacked on this branch so each review sees the connector behavior its own change depends on; once this merges, retarget #184 to dev.

• Upstream bodies are capped before parsing (4 MiB success / 64 KiB error), and streams close on every exit. Overflow stays an explicit error; an oversized or slow error body keeps its real HTTP status.
• One deadline per request: connect, headers and body share a single monotonic budget, so a slow-but-alive upstream can no longer double the aligned HTTP timeout past the connector Lambda's 60s wall.
• Connection tests for Prometheus, Mimir, Loki, Tempo, Jaeger, Dynatrace and ClickHouse run the real bounded query operation; Datadog checks metric-query access, not key validity alone. Every probe returns {ok, verified, latency_ms, error?} with fixed sentences, accepts ok|empty|partial (an upstream warning on vector(1) is still access), and keeps the SSRF-policy classification.
• Jaeger, Dynatrace, Datadog and Loki report partial / unverified completion states instead of claiming absence. Loki and Jaeger report reaching the requested limit as limitSaturated + partial with requestedLimit/returnedCount; truncated is reserved for records the connector actually dropped (Jaeger's byte-bound preview is no longer echoed).
• Prometheus and Mimir enforce the saved execution ceiling inside the connector, with aligned HTTP deadlines. Mimir's Prometheus API prefix is read from apiPrefix (null/absent = /prometheus, explicit "" = root).
• The default instance's kind mirror is stamped with its instance id (mirrorDefaultCredential(..., instanceId), all four write sites). The connector's id-keyed lookup accepts the mirror only for that default; a sibling id can never borrow it. A mirror written before the stamp (migrated default whose id key was never written) fails closed on the credential-blind path; the administrator save of the verified current default (exact endpoint match, under the manage lock) recovers it, writes the id key and stamps the mirror. Switching or deleting a default never lends the previous default's mirror to its successor. Rollout order and the backfill procedure: docs/runbooks/datasource-credential-isolation.md — re-save every migrated default before deploying the connector producers.
• timeoutS rides the console conn config for every kind (previously ClickHouse only), so console and credential-blind paths hand the connector the same bound.
• New jaeger_vpc_enabled / dynatrace_vpc_enabled (default false, require agentcore_enabled && integrations_enabled).

No deployment or flag activation is done here. A gateway catalog/schema refresh is a separate provisioning step.

Review follow-ups (round 1)

Lambda wall-clock overrun → single deadline; migrated-default NotConnected regression → stamped mirror, recovery only through the verified-current-default admin save, documented backfill before rollout; timeoutS path divergence → forwarded on the console path here; Loki saturation reported as complete → limitSaturated; pinned_http.py asserted but absent → that docstring now lands with the module in #184 and the resolve-and-recheck disclosure is kept here; latency_ms restored; Prometheus/Mimir probe acceptance aligned; Mimir apiPrefix null handling; orphaned _timeout_param helpers removed.

Tests

• bash scripts/v2/merge-verify.sh on Python 3.12 against dev 9b8ab40: web vitest PASS, Terraform fmt + validate PASS, deployment Node tests PASS. The only Python-stage failures are test_review_codec_sandbox.py / test_pr_review_head_images.py, which need a prepared AWSOPS_REVIEW_CODEC_STATE this host does not have (same on the dev baseline).
• Connector suites: 279 passed (test_external_connections.py adds deadline-sharing, Loki/Jaeger saturation, probe latency/SSRF-classification and stamped-mirror cases).
• terraform test (mock provider) → 27 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

Both panels' MAJOR claims check out as either intentional, disclosed behavior changes or as heuristics that exactly mirror existing base-code logic. Key verifications: the backfill script's origin-match condition replicates manage/route.ts:188-194's existing "round-9" defense (and adds stricter is_default + slug-named-row conditions), the root CLAUDE.md itself cites "ADR-007/039" (so codex's invalid-ADR finding is a false positive), and the ClickHouse output-size exposure predates this PR (base had no byte bound at all; the PR adds the first input cap). Here is the synthesized chair review.

---

Chair Review — PR #183: fix(datasources): bound connector reads and verify real query access

1. Summary

This PR replaces public health-endpoint connection tests with bounded real-query probes across all eight native observability connectors, adds byte/deadline bounds to connector HTTP reads, makes explicit-instance credential lookups fail closed (no kind-mirror fallback), and extends the completeness-evidence (collectionStatus) contract to Datadog/Dynatrace/Jaeger/Loki. Both panels independently verified that no AWS-mutating operation, IAM change, or ADR-005 boundary crossing is introduced. All panel MAJORs were reconciled against BASE + patch and found to be either deliberate, changelog/docs-disclosed behavior changes with per-datasource knobs, pre-existing exposure, or heuristics identical to existing accepted base-code behavior — none survives as a gate-blocking defect.

2. Issues per lens

L2 — Code correctness

MAJOR claims — verified, downgraded (excluded from gate):

• Absolute-deadline timeout contract (claude flagged MAJOR; codex did not flag). http_json now enforces one monotonic deadline across connect+headers+body, so a large legitimate body on a slow link (e.g. wide loki_query_range) that previously streamed under per-operation timeouts can now abort. Verified as intentional: the bounding is the PR's stated purpose, disclosed in the changelog ("provider response bytes and body reads are bounded") and all four docs locales, and timeoutS (up to 55s) is the admin knob. Intentional tradeoff, not a defect — downgraded to a behavior-change note.
• Prometheus/Mimir now always send a server timeout, defaulting to 10s on every path (claude flagged MAJOR). Previously no timeout param was sent unless the caller supplied one, so heavy PromQL relying on the server default (often 2m) will now fail until timeoutS is raised. Verified as the PR's headline, disclosed change (changelog: "Prometheus and Mimir default to a 10-second execution ceiling, configurable per datasource") with matching docs in four locales and updated tests. Product decision, not a bug — downgraded to a rollout note.
• ClickHouse response can exceed Lambda's 6 MB sync limit (codex flagged MAJOR; claude independently found it but rated MINOR). Verified pre-existing: base http_json had no byte bound at all and _run_sql/ok() never byte-bounded output; the PR adds the first input cap (8 MiB) and bounds rows via max_result_rows. Not a regression introduced by this diff — excluded from the gate per the out-of-scope rule, retained as MINOR below.

MINOR:

• (both panels, convergent) ClickHouse tool output still has no byte bound (unlike _byte_bound/_bounded_result elsewhere); a wide row set admitted under the new 8 MiB input cap can exceed Lambda's 6 MB response limit. Pre-existing, but this PR raises the admitted input size, making it more reachable.
• (codex) loki_mcp._requested rejects non-canonical limits like "001" ([1-9]\d{0,8}) but still sends them upstream; if Loki returns exactly that many lines, saturation goes unrecorded and collectionStatus can read ok. Verified against the diff — real, narrow.
• (claude) loki_mcp._query_response drops resultType: "scalar" payloads as unknown (base passed them through) — fail-closed but lossy for literal LogQL expressions.
• (claude) jaeger_health inherits jaeger_search's strict span validation; a healthy Jaeger serializing "data": null for an empty search, or with float startTime spans, reports ok: false on the connection test.
• (claude) _OPEN_POOL is a module-global 4-worker pool with no per-invocation reset; four stuck opens (sockets that ignore shutdown()) permanently starve a warm container.
• (claude) On the connectors' Python 3.11 runtime, concurrent.futures.TimeoutError is TimeoutError, so an in-worker socket timeout is relabeled "header deadline exceeded" in logs; behaviorally equivalent, diagnostically misleading. Also, _read_bounded discards a body that completes exactly at the deadline.
• (claude) backfill-datasource-instances.mjs's WHERE name = $1 AND kind = $1 silently produces neither a created nor a skipped entry when a row named after a kind exists with a different kind — safer than base but gives the operator no signal. Dead max(1, …) and a duplicated collectionStatus ternary in jaeger_search.

L3 — Security / AWS mutation safety

Both panels independently confirmed: no ADR-005 violation — no AWS-mutating call, no IAM/Terraform change, no hardcoded secret; SSRF guards (assert_host_allowed before every request, _NoRedirect, ProxyHandler({}), IP pinning with intact SNI/cert verification through the new tracking handlers) are preserved; the Mimir apiPrefix is path-only and regex-bounded.

MAJOR claims — verified, downgraded (excluded from gate):

• Fail-closed strict load_datasource with no in-code deployment gate (claude flagged MAJOR). A migrated default whose map[<id>] key was never written raises NotConnected on every credential-blind explicit-ID path after rollout. Verified as the deliberate security fix: fail-open mirror fallback is the vulnerability being removed (a sibling inheriting the default's credentials). The required ordering (backfill + admin re-save before producer deploy) is documented in the new runbook, the changelog, and enforced by this repo's established runbook-ordered-deployment pattern (same as migrations-before-promotion in CLAUDE.md). Operational rollout prerequisite, not a code defect — downgraded to MINOR.
• Backfill origin-match can bind another instance's credentials (codex flagged MAJOR). Verified against base manage/route.ts:188-194: the origin-equality heuristic is byte-for-byte the repo's existing "round-9 defense in depth" for trusting the kind mirror, and the backfill is stricter (also requires is_default, the slug-named row, and row-creation-or-origin-match). In codex's same-origin/different-tenant scenario, base behavior already served that exact mirror blob to that row at read time via the fallback this PR removes — the backfill materializes the status quo, never widens it, and the runbook discloses the residual and mandates re-save. Equal-or-better than base — downgraded to MINOR.

MINOR:

• (both panels touched this) Same-origin is not credential identity for multi-tenant same-host setups (shared gateway, different org_id); the residual is documented but the heuristic could require an exact-blob or ID-stamped match as the runbook's future "ID-stamped mirror and 409 guard" contract describes.
• (claude) Datadog hand-edited Secrets Manager blobs with apiKey/appKey and a UI-stamped authType (including 'none') silently stop sending keys — a hard break the module docstring previously advertised; the runbook documents the custom-header workaround, but the changelog undersells it.
• (claude) apiPrefix is consumed from the credential blob but absent from CRED_KEYS/sanitizeDsSettings/any UI field — only settable by hand-editing the secret or transiently via /api/datasources/test, contradicting the manage-route principle that connection config comes through validated fields.
• (claude) query_probe's fixed-sentence errors drop the HTTP status an admin previously saw; a coarse non-echoing category (auth vs. transport vs. 5xx) would preserve diagnosability without echoing provider text.

L4 — Observability / data-integration correctness

No Steampipe query or CIS control is touched. Both panels found the completeness-evidence model coherent: ObservedPayload keeps transport status out of upstream-forgeable JSON, oversized/slow bodies can never read as "empty", limitSaturated/requestedLimit/returnedCount distinguish saturation from loss, and Tempo's bare_empty acceptance is correctly scoped to the health probe only (pinned by test).

MINOR:

• (claude) Marker asymmetry: loki_mcp.err() carries collectionStatus: "error" but jaeger_mcp/datadog_mcp/dynatrace_mcp err() still do not, despite their success paths now emitting the field.
• (codex) The Loki "001" limit gap above also has an L4 face: a saturated result can be marked complete.
• (claude) dynatrace_query reuses MAX_SERIES (50) for the inner dimension list, indistinguishable from the outer-series cap in partial evidence.
• (claude) Prometheus/Mimir vector(1) probes evaluate purely in the query engine; "tenant-scoped query access" overstates what they prove for a tenant with API access but no series-read permission (Loki/Tempo/Jaeger/ClickHouse probes do exercise real read paths).

L5 — Docs/ADR consistency

The four locale docs, api-reference.md, and the new runbook were verified consistent with the implemented probes, the 55s ceiling, the 10s Explore cap, and the existing base behavior of manage/route.ts that the runbook describes.

• Codex's "ADR-039 is invalid" MINOR is a false positive — the root CLAUDE.md itself cites "ADR-007/039" (custom-MCP governance section); ADR numbers carry legacy aliases in this repo (e.g. ADR-002[legacy 042]), and ADR bodies intentionally live in the private upstream. Rejected.
• Codex's "new Korean text in docs/" MINOR is weak — the edited api-reference.md cell was already Korean in an existing bilingual body; CLAUDE.md's scoped rule treats such bodies as migration backlog, not a violation trigger for in-place amendments. Not adopted into the gate; noted as a judgment call (a fully-English rewrite of the cell would have been cleaner).

MINOR:

• (both panels, convergent) The rewritten connection-test tables in all locales list seven kinds and omit Mimir, whose mimir_health probe this very PR rewrites.
• (claude) "Schema/discovery calls keep the connector's fixed HTTP budget" is inaccurate for Tempo tag discovery and dynatrace_problems, which now inherit request_timeout(creds) — an admin setting timeoutS: 1 shortens discovery to 1s.
• (claude) Stale comments: web/lib/datasources.ts:16-22 (old Prom/Mimir Explore-only claim) and web/app/api/datasources/query/route.ts:63-70, 83-88 (ClickHouse-only timeoutS, "Loki accepts no per-request upstream timeout").
• (claude) Changelog/docs omit two user-visible changes: timeoutS now rides the conn config for every kind, and Mimir's new apiPrefix + endpoint-path de-duplication (undocumented in all four locales).
• (claude) datasource_http.health() is now unused by all connectors but retained and still tested — a second, weaker probe mechanism alive beside the documented contract.

3. Suggestions

1. Byte-bound ClickHouse tool output (reuse the _byte_bound pattern) before returning, now that 8 MiB inputs are admitted.
2. Canonicalize the Loki limit before sending (parse → re-stringify), so _requested and the upstream request can never disagree.
3. Add collectionStatus: "error" to err() in jaeger/datadog/dynatrace connectors for producer-set consistency.
4. Either add apiPrefix to the validated settings surface (sanitize + UI) or reject it in /api/datasources/test's unfiltered conn-config spread.
5. Add the Mimir row to the connection-test tables (all locales), fix the "fixed HTTP budget" sentence for Tempo/Dynatrace discovery, and refresh the two stale web comments.
6. Make the backfill log kind-mismatch rows (currently silent) and consider tightening the mirror-copy condition to an exact-blob match ahead of the planned ID-stamped-mirror contract.
7. Amend the changelog entry to cover the every-kind timeoutS forwarding and the Datadog no-authType hard break explicitly.

4. Verdict

All CRITICAL/MAJOR candidates were reconciled against BASE plus the complete patch: the two claude L2 MAJORs are disclosed, knob-controlled intentional behavior changes; codex's ClickHouse L2 MAJOR is pre-existing exposure that this PR actually narrows; claude's L3 MAJOR is the deliberate fail-closed fix with a documented, repo-conventional rollout ordering; codex's L3 MAJOR replicates the base code's own accepted heuristic under stricter conditions and never widens base exposure. No in-scope CRITICAL/MAJOR remains; remaining findings are MINOR. No images were listed in the manifest and no deleted/unavailable/omitted entries exist.

IMAGE_COVERAGE: NOT_REQUIRED

---

Triggered by commit 5561741f9e0ef563ef7b66a7302e5707e08c4693 · workflow: .github/workflows/pr-review.yml

[Atom-oh]

Moved agent/lambda/test_external_connections.py into the stacked follow-up #191, bringing this diff from 131 KiB down to 104 KiB, under the AI review input limit. The runbook's pytest command works once both PRs merge.

[Atom-oh]

Review response for a1c086b8:

• Backfill credential binding. Fixed. The kind key mirrors only the current default, so the backfill now copies it only into the kind-named row when that row is the default. It reports other rows as skipped, to be re-saved through the admin flow.
• CHANGELOG. Fixed. The connector entry was meant to ship with the web contract PR, but that PR merged without it. It is now restored in both language sections.
• Timeout docs vs. the Explore 10s cap. Fixed in all four locales and docs/api-reference.md. The Explore cap is intentional: the CHANGELOG entry says the ceiling is "tightened by Explore".
• Datadog authType: none dropping apiKey/appKey. Kept as intended. An explicit none sends no stored credentials. This matches the manage route's KEEP_BY_AUTH.none = [], and test_explicit_no_auth_cannot_transmit_legacy_datadog_keys in test(datasources): add connection identity and bounded I/O regressions #191 locks it in. The runbook now explains how to re-save a legacy Datadog blob with Custom header authentication.
• Transport-layer tests. _read_bounded, the deadline watchdog, _seconds, query_probe, the Loki _query_response states and the sentinel probes are covered by the 121 tests in test_external_connections.py. That file moved to stacked test(datasources): add connection identity and bounded I/O regressions #191 only to keep this diff under the review input limit. It passes against this head.