[Atom-oh]

Summary

Part 1 of 4 split from #227 (six-pillar discovery diagnosis), merged in order A → B → C → D. This part has no worker diagnosis code.

• Trace graph: CLIENT database spans record bounded per-edge instance↔database observations (deduplicated per pair, stored order, cap disclosed per edge as omitted/unbound counts); fractional Tempo timestamps bind.
• BoundedTtlCache for NFM/DNS-log reads (LRU 128, 32 in-flight, 90 s load deadline) with SDK request timeouts; saturation or deadline returns a retryable 503 instead of a wrong 404.
• Agent tool contracts: bounded/validated CloudWatch get_metric_data/get_metric_metadata, Compute Optimizer rows with provider guards and utilization evidence, flow-log samples disclose window/limit/truncation, Tempo trace budget; catalog schemas use only SchemaDefinition keys; chat prompt rules for network/cost tools.
• web/lib/CLAUDE.md documents the bounded cache.

Test plan

• web: tsc 0 errors, vitest 5648 passed
• agent/lambda 991, agent 106, agentcore 94, workers 1752 passed

🤖 Generated with Claude Code

[github-actions]

🤖 AI Code Review (two independent reviewers)

Reviewer responses: batch 1/2: codex/ALL claude/ALL; batch 2/2: codex/ALL claude/ALL

Status: PASSED — No blocking issues found

Reviewed in 2 complete batches split at file boundaries; every batch must pass.

Batch 1/2 — PASS

All gate-relevant claims are now verified. The one panel MAJOR (missing dimensions/unit echo in get_metric_data) does not survive verification as blocking: the handler validates and either applies the dimensions/unit or returns a 400 — it never silently drops them — and the MCP consumer retains its own tool-call arguments in context, so there is no resulting-code failure trigger where the response misrepresents the series. Codex's Compute Optimizer metric-name finding is a confirmed false positive against the real service model enums. Final synthesized review:

---

PR #242 — Chair Review (Batch 1 of 2): trace instance observations and bounded observation reads

Summary

This batch hardens the AgentCore read-only tool surface (validated CloudWatch metric requests, Compute Optimizer evidence projection with rank/freshness/missing-metric disclosure, bounded Tempo instance-identity claims, flow-log truncation disclosure), fixes a real KeyError in the flow-monitor Lambda's wrapped-MCP argument path, and converts four web API routes to distinguish transient saturation (retryable 503) from genuine "unknown resource" 404s. All new AWS operations are read-only; no IAM, Terraform, or ADR-005-relevant change appears. After chair verification, no CRITICAL or MAJOR finding stands; the one panel MAJOR was downgraded on verification (details below).

Issues per lens

L2 — Code correctness

No CRITICAL/MAJOR. Confirmed-correct fixes both panels accepted: flowmonitor.py params['vpc_id'] → args['vpc_id'] (verified against BASE — the wrapped {"tool_name","arguments"} envelope previously raised KeyError), metric_value() returning None instead of fabricating 0, datetime.now(timezone.utc) replacing naive utcnow(), bool-rejection via type(x) is not int, and the len(events) >= 50 truncation term (necessary because filter_log_events(limit=50) can never trip cap_list's > limit check).

• MINOR (claude only) — analyze_metric (agent/lambda/aws_cloudwatch_mcp.py:76-101, unchanged) retains every defect this PR fixes in its sibling branch: unvalidated dimensions.items() (non-dict string → AttributeError → 500), utcnow(), and d.get(stat, 0) zero-fill. Verified present in BASE. Out of diff scope — excluded from the gate; worth routing through _metric_request/metric_value as a follow-up.
• MINOR (claude only, verified) — get_vpc_flow_logs (network_mcp.py:373) echoes windowMinutes but the minutes input is still unvalidated (a string "30" → TypeError), and catalog.py leaves the tool's inputSchema at {vpc_id} only, so via the Gateway the window is always the default 30 and the new "qualify conclusions by sample/window" prompt rule cannot actually be exercised with a chosen window.
• MINOR (claude only) — _ranked_option ("provider rank is authoritative") is applied to EC2/RDS but ECS and Lambda recommendations keep options[0], half-applying the stated invariant.
• MINOR (claude only) — Tempo _trace_attributes pop-then-reinsert means a valid-then-malformed duplicate instance key now emits no claim at all (BASE kept the last entry) and instance keys reorder to the end. Fail-closed and likely intended, but untested behavior change.
• Unverified cross-batch dependency (both panels) — BoundedTtlCache/CacheUnavailableError/isTransientRead/abortableSleep are imported by five batch-1 files but web/lib/bounded-cache.ts ships in batch 2 (listed path). Non-blocking per the cross-batch rule; batch 2 must confirm the implementation matches the extensive contract tests shipped here.

L3 — Security / AWS mutation safety

Both panels independently agree: no ADR-005 violation, no IAM widening, no secrets. Chair-verified: every touched AWS call is read-only (DescribeFlowLogs, GetMetricStatistics, ListMetrics, FilterLogEvents, Compute Optimizer Get*Recommendations), all already exercised by unmodified code in the same handlers, so the absence of a Terraform IAM change is correct. verifyUser remains the first statement in all four routes; cross-account access stays exclusively on get_role_arn/get_client, and the new flowmonitor contract test pins that boundary. The Tempo instance-claim sanitization (512-char, control-char-free stringValue only, no arbitrary AnyValue retention) is a hardening, not a new surface.

• MINOR (claude only) — raw SDK error text (e.message) still reaches authenticated clients on the analytics/query-step 502 paths and the onboarding-200 bodies this PR did not harden (dns-logs/analytics/route.ts, nfm/query/route.ts final catch; dns-logs/route.ts, nfm/route.ts 200 bodies). The PR redacts one await earlier but not the adjacent one, and the new tests pin the leaky behavior. Pre-existing, behind auth — non-blocking.
• MINOR (claude only) — the Tempo inline comment "never retain arbitrary AnyValue maps/extra attributes" is true only for the three instance keys; the other ~20 allow-listed keys still echo the full source entry (byte-budget-bounded). Scope the comment or extend the rebuild.

L4 — Observability / data-integration correctness

• Downgraded from MAJOR to MINOR (claude flagged MAJOR; codex did not flag) — get_metric_data enforces dimensions/unit but does not echo them in the response. Chair verification: _metric_request always includes validated Dimensions and either applies a valid Unit or returns a 400 — it never silently drops either — and the MCP consumer has its own tool-call arguments in context, so no resulting-code failure trigger exists where the response misrepresents the series read. Echoing them is still a worthwhile one-line evidence-completeness improvement (and would make an old-Lambda rollout window detectable).
• REJECTED (codex-only MINOR) — "the I/O classifier misses standard camel-case EbsReadOpsPerSecond/DiskReadOpsPerSecond names." Verified against the Compute Optimizer service model: the EC2 MetricName enum is exactly Cpu, Memory, EBS_READ_OPS_PER_SECOND, DISK_*, NETWORK_* (uppercase-underscore), and the RDS RDSDBMetricName enum uses the EBSVolume*/StorageNetwork*/ReadIOPSEphemeralStorage forms the PR matches, with EBSVolumeStorageSpaceUtilization and VolumeBytesUsed correctly excluded as capacity. The camel-case names do not exist. False positive.
• MINOR (codex only, valid) — flow-log messages are truncated to 200 characters per message, but the new truncated flag describes only count/pagination loss; a single longer record returns truncated: false despite losing fields. Surface per-message truncation.
• MINOR (claude only) — split-workflow rollout: the gateway schema (provision) and Lambda code (Terraform) deploy via independent workflows; schema-first lets the model send statistic: "p95"/unit to the old Lambda (API error / silently ignored unit). The repo already has a Terraform-first convention for exactly this (docs/reference/05-agentcore.md) — state it for this change.
• MINOR (claude only) — missingUtilizationMetrics tracks only cpu/memory/io, while the new agent.py cost rule also gates on "DB connections/latency"; an RDS record lacking DatabaseConnections reports no missing evidence. Adding a connections key for RDS would align tool output with the prompt gate.
• MINOR (claude only) — utilizationTruncated/projectedUtilizationTruncated are True when the provider field is simply absent (verified in the diff: not isinstance(value, list) or len(projected) != len(records)), conflating "absent" with "cut". Fail-closed direction, but the name misdescribes the evidence; *Unknown/*Absent would read correctly.
• MINOR (claude only, verified) — web/lib/CLAUDE.md documents a 90 s per-load deadline, but nfm/query and dns-logs/analytics declare maxDuration = 60 (verified in BASE), so on runtimes enforcing it a hung loader hits the platform timeout before the designed 503.

L5 — Docs / ADR consistency

No blocking inconsistency; codex found none, claude confirmed the ADR-005 boundary is uncontradicted, the gateway-tool-catalog.json name-only mirror is unaffected by description/schema changes, the new schema nodes satisfy catalog_check.py's key invariants, and the corrected agent.py tool chains all reference registered catalog tools (now test-enforced by the new routing-contract test).

• MINOR (claude only) — docs-site AI-assistant guides (4 locales) still list analyze_reachability/query_flow_logs with exactly the semantics this PR corrects; those paths are in neither batch, so published docs will disagree with the shipped prompt. Out of diff scope — excluded from the gate; follow-up needed.
• MINOR (claude only) — docs/reference/05-agentcore.md's completeness-contract table is not extended with the new disclosure fields (windowStart/End, windowMinutes/limit, the seven finops evidence fields) and is not in either batch.
• MINOR (claude only) — the new benchmark (scripts/v2/benchmark-observation-cache.mjs) is documented in web/lib/CLAUDE.md but invoked by no CI stage, and it omits the loadTimeoutMs option the same doc's new rule requires; its direct .ts import ties it to Node type-stripping behavior with nothing exercising it.
• MINOR (claude only) — the explicit jsonschema==4.26.0 pin lands in a shared pip install with agent/requirements.txt (mcp/strands); a future conflict hard-fails the merge job. The explanatory comment is correct; note the two-file coupling.

Suggestions

1. Echo Dimensions and Unit (or unit: null) in the get_metric_data response — one line, closes the evidence-scope gap and makes a stale-Lambda rollout window self-detecting.
2. Route analyze_metric through _metric_request/metric_value, and add minutes (validated) to the get_vpc_flow_logs catalog schema so the new window disclosure is actually steerable.
3. Extend _ranked_option and _recommendation_evidence to ECS/Lambda, add an RDS connections coverage key, and rename *Truncated → *Unknown for the absent-field case.
4. Rename or split the flow-log truncated flag to disclose per-message 200-char truncation.
5. Follow-ups outside this PR: redact the remaining raw-SDK-error paths in the four routes, update the docs-site tool tables and the 05-agentcore completeness table, wire the benchmark into a CI stage (or mark it manual-only), and state the Terraform-before-provision ordering for this change.

Verdict

No CRITICAL or MAJOR finding survives verification in this batch. The sole panel MAJOR was downgraded with a concrete resulting-code reconciliation, and one codex MINOR was rejected against the AWS service model. All remaining findings are MINOR or out of diff scope. The cross-batch bounded-cache.ts dependency is non-security and listed in batch 2; batch 2's review must confirm it. The image manifest is complete with zero images and zero unavailable/omitted entries.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 2/2 — PASS

Both panels' MAJOR candidates are now verified against BASE. Claude's performance MAJOR runs in a default-off background rebuild bounded by existing caps (1000 spans × 4000 nodes), and codex's orphaned-query MAJOR is pre-existing BASE behavior that this diff strictly improves — I'm downgrading both to MINOR with the reasoning recorded below. Here is the synthesized chair review:

---

PR #242 — batch 2/2 chair review

Summary

This batch replaces the unbounded TTL/in-flight cache in dns-logs.ts/nfm.ts with a new bounded, abortable BoundedTtlCache, adds abort/stop plumbing to the Logs Insights and NFM async-query pollers, and introduces inventory-validated EC2-instance→RDS observations on trace queries edges with strict identity/scope binding, caps, and disclosure counters, plus matching docs. Both panels found the security posture sound and the core logic correct; the surviving findings are efficiency, cleanup-hygiene, and doc-precision issues, none of which block the batch. Image manifest is empty and complete, so no pixel evidence was required.

L2 — Code correctness

Both panels confirmed the subtle parts of the new cache are correct: the generation guard plus the pending.get(key) === promise identity check prevent a pre-clear() load from writing stale values or detaching a replacement load; TTL expiry is exclusive and not extended on access (matching the 239 999/240 000 ms tests); eviction cannot spin; abortableSleep has no TDZ hazard or listener leak; Tempo ns→ms stays within safe-integer range; pair dedupe is input-order independent via the (observed_at_ms, correlation_id) tie-break; and the edge() signature change is safe for all other call sites.

• MINOR (downgraded from one model's MAJOR; the other model found no L2 issue) — observedInstanceDatabasePair in web/lib/trace-graph.ts re-filters all infraNodes (three passes, with exactEndpoint label regexes on scoped RDS nodes) for every CLIENT db span carrying an EC2 claim. Chair verification: worst case is TRACE_SPAN_CAP = 1000 spans (web/lib/graph-store.ts:45) × the 4 000-node published-graph ceiling (graph-store.ts:211), i.e. up to a few million synchronous iterations. However, this runs only inside rebuildTraceGraph, invoked from the periodic rebuild in web/instrumentation.ts (default off, GRAPH_REBUILD_INTERVAL_MINS) and the scripts/v2/graph-rebuild.mjs CLI, after the infra SELECT transaction is released — no DB lock is held, no request path is traversed, and output is correct. A bounded sub-second event-loop stall in a default-off background job is an efficiency defect, not a merge-blocking bug. Fix is cheap: build two account|region-keyed Maps (instance ID → {count,id}, normalized endpoint → {count,id}) once per buildTraceGraph call, preserving the exactly-one-match semantics.
• MINOR (one model) — in web/lib/nfm.ts, the try/catch that calls stop() wraps only the status-poll loop; a deadline abort during the GetQueryResultsMonitorTopContributors pagination loop throws without stopping the query. Low impact (the query already reached SUCCEEDED), but the stated intent in the diff's own comment isn't met for that window. Move the pagination loop inside the same try.
• MINOR (one model) — maxInFlight: 32 in dns-logs.ts permits up to 224 concurrent Insights StartQuery calls (7-query fan-out per load) against the default per-account limit of 30 concurrent Insights queries. Net improvement over BASE (which had no bound at all), and the overflow degrades via throttling that isTransientRead classifies; still, a lower bound for this module would match the quota.
• MINOR (one model; pre-existing) — a transiently fully-failed dnsAnalytics/coreDnsAnalytics read still caches an empty payload for the full 4-minute TTL. BASE behaved identically and the result discloses failed[]; out of gate scope as a non-regression.
• Unverified cross-batch dependency (not blocking) — isTransientRead and CacheUnavailableError are exported but unconsumed in this batch; the listed batch-1 paths web/app/api/dns-logs/route.ts, web/app/api/dns-logs/analytics/route.ts, web/app/api/nfm/route.ts, web/app/api/nfm/query/route.ts plausibly consume them, and the listed web/lib/bounded-cache.test.ts covers the new module. If no route maps CacheUnavailableError to a retryable response, saturation would surface as an opaque 500 — verify in batch 1.

L3 — Security / AWS mutation safety

No findings — both models agree, and chair verification concurs. Every AWS call in this batch is read-only or a self-initiated query-lifecycle call (ListResolverQueryLogConfigs, DescribeLogGroups, StartQuery/GetQueryResults/StopQuery, NFM ListMonitors/ListScopes/Start|GetQueryStatus|GetQueryResults|StopQueryMonitorTopContributors), all already present in BASE and already granted in terraform/foundation/workload.tf; the diff adds zero IAM surface and no mutation. ADR-005's frozen boundary is intact. No hardcoded secrets (the password=secret strings in trace-source.test.ts are negative-assertion fixtures proving redaction). The new telemetry→inventory attribution path is defensively built: identityClaim bounds and control-char-filters the three new resource attributes, only validated i-…/EC2-ARN forms ever reach persisted meta, cross-account binding requires a literal 12-digit account with 'self' resolved only through a validated trustedHostAccountId, conflicting ARN scope is rejected outright, and correlation_id is a SHA-256 that never discloses the raw trace ID.

L4 — Observability / data-integration correctness

• MINOR (downgraded from one model's MAJOR; the other model flagged only the narrower NFM-pagination variant) — a non-abort polling failure (new 10 s SDK requestTimeout, throttling, transient GetQueryResults error) in dns-logs.ts/nfm.ts releases the cache slot while the server-side query keeps running, unstopped. Chair verification against BASE web/lib/dns-logs.ts:72-82: BASE had no error handling around the poll loop either, so this orphaning is pre-existing behavior, not a regression — the diff strictly adds cleanup (abort-path StopQuery) that BASE lacked. Impact is bounded: the orphaned query completes server-side on its own (its scan would have been billed on success anyway), the cost is transient occupancy of the concurrent-query quota, and the DNS degrade path's 4-minute cache prevents retry storms. Recommended fix stands: stop the query on every non-terminal polling exception where a queryId exists, not only when abortSignal.aborted.
• MINOR (one model) — pair-cap truncation (instance_observations_omitted) is edge-local and never degrades topology_graph_state.status to partial, unlike every other truncation signal in the pipeline (nodeDrops/edgeDrops/orphanSpans at graph-store.ts:363-366); and sql_reader.topology_edges cannot see either field (chair-verified: the view projects only numeric spanCount/metricCount, migration 01M279W0J9…:55-62). Internally consistent and documented, so a disclosure asymmetry rather than a false-evidence bug.
• MINOR (one model) — db node identity uses raw span.dbHost (case-sensitive, trace-graph.ts:114) while pair binding lowercases via exactEndpoint; mixed-case telemetry splits one logical pair across two edges with divided observation_count.
• MINOR (one model) — exactEndpoint rejects host:port forms and buildInfraGraph skips EC2 rows lacking network context, so the single instance_observations_unbound counter conflates malformed telemetry with genuine inventory gaps, limiting its diagnostic value.

Confirmed unchanged by the diff: Insights query strings, BIN_SEC binning, query windows and limits; NFM_MAX_RANGE_SEC validation still precedes cache admission; poll caps unchanged.

L5 — Docs/ADR consistency

• MINOR (one model; chair-verified) — instance_observations_unbound contract gap: a CLIENT span with dbSystem but no dbHost still creates a queries edge (trace-graph.ts:113-129), yet a valid EC2 claim on it returns undefined (first guard in observedInstanceDatabasePair) and is never counted unbound, while the API reference's wording ("no unique inventory … RDS instance endpoint") implies it would be. Either count it or narrow the doc.
• MINOR (one model) — docs/runbooks/agent-sql-reader.md:353-357 ("the current database writer does not populate every allowed identity key") is now stale: this diff makes the db-node writer populate accountId/region, which the node view allowlists, so they now appear on class='trace' AND kind='db' rows. Freshness nit, not a contract break.
• MINOR (one model) — the new shared web/lib/bounded-cache.ts primitive has no doc entry in this batch; web/lib/CLAUDE.md is changed in batch 1 and plausibly covers it (unverified cross-batch dependency — note BASE web/lib/CLAUDE.md currently documents the old "TTL cache + in-flight dedupe" pattern for nfm.ts/dns-logs.ts, so that update is expected there).

Verified consistent: both new doc anchors resolve bidirectionally; the runbook's sql_reader.topology_edges projection claim matches the migration; the API-reference field table matches the implementation (CLIENT-only, 200-pair cap, latest-span correlation_id, provenance: "trace_span"); the Aurora-cluster/RDS-Proxy "stays unbound" caveat is accurate given exactEndpoint has no prefix fallback. No CHANGELOG rule violation in this batch (no PR/round numbers, no duplicate bullets added).

Suggestions

1. Index infraNodes once per buildTraceGraph call into account|region-keyed instance/endpoint Maps to replace the per-span filters.
2. In both pollers, call the stop command on any non-terminal polling exception with a known queryId (not only on abort), and move NFM's result pagination inside the stop-guarded try.
3. Lower dns-logs.ts's maxInFlight (or sub-limit the 7-query fan-out) toward the 30-concurrent Insights quota.
4. Normalize dbHost (lowercase, strip trailing dot) before composing the db node name so case variants collapse to one edge.
5. Reconcile the instance_observations_unbound doc wording with the missing-dbHost and non-CLIENT cases, and refresh the stale runbook sentence about db-node identity keys.
6. Consider adding instance_observations_omitted to the (edge-local vs. collection-status) note in the API reference.

Verdict

No CRITICAL findings. Both panel MAJORs were downgraded on chair verification: the quadratic scan is bounded by existing caps and confined to a default-off background rebuild (efficiency, not correctness), and the orphaned-query cleanup gap is pre-existing BASE behavior that this diff partially fixes rather than regresses. All remaining findings are MINOR. Security review is clean with both models in agreement: read-only AWS surface, no IAM change, no secrets, ADR-005 intact.

IMAGE_COVERAGE: NOT_REQUIRED

---

Triggered by commit 40cc839ace8bdaac1f2ecc6762e777ea294f2d18 · workflow: .github/workflows/pr-review.yml