[Atom-oh]

AI Diagnosis gains bounded, default-off Application Signals service discovery in the existing Fargate evidence loop. It supplements inventory, DNS, X-Ray and Network Flow Monitor evidence for six-pillar analysis.

Service-to-DB observations retain logical service identity. Compute membership and DNS endpoint aliases remain configuration; typed inventory identifiers and verified member sessions prevent native call fan-out. Isolated invalid rows leave explicit gaps without discarding valid scoped evidence. Batch failures disqualify prior rows and remove relationships citing failed managed evidence. Every page request checks the remaining analysis budget against the configured HTTP timeout; deadline stops do not appear as empty catalogs. Managed nodes have their own resource share and are evicted before native trace evidence.

CloudWatch Omni remains a private, directly tested adapter with no production activation path. Terraform rejects every nonempty diagnosis_omni_spaces map, and the worker receives no Omni environment wiring or credential-vending permission. Activation requires a separately reviewed, enforceable read-only per-space IAM boundary.

Validation:

• Python diagnosis/runtime-policy/Terraform-read contracts: 1,434 passed, 36 pre-existing skips, 194 subtests passed.
• Managed adapter regressions: 82 passed, including locked SDK contracts and production Omni-hold tests without AWS calls.
• Terraform 1.17.1 validation and mock runtime IAM tests: 52 passed.
• Python compilation, Git diff and Terraform formatting checks passed.

Latest-HEAD AI review and required checks must pass before review completion. No flags were activated and no AWS deployment was performed.

[github-actions]

🤖 AI Code Review (two independent reviewers)

Reviewer responses: codex/ALL claude/ALL

Status: PASSED — No blocking issues found

Both panels' load-bearing claims check out against the base tree: local.runtime_read_condition exists and conditions the new IAM grant; EvidenceIndex refuses resource_binding: "unresolved" evidence for observation claims (which bounds the Omni freshness finding); and scoped_clients.member_factory builds clients generically, so the member Application Signals path works. The one panel MAJOR (global vs per-region managed-resource cap) is mechanically accurate but describes behavior the PR's own docs, plan, and tests deliberately specify — I'm downgrading it with rationale below.

PR #249 — feat(diagnosis): add bounded Application Signals discovery — Chair Review

IMAGE_COVERAGE: NOT_REQUIRED

(The manifest lists no images, deletions, or unavailable entries; both panel cells declared NOT_REQUIRED.)

1. Summary

This PR adds a default-off, flag-gated (diagnosis_managed_discovery_enabled) bounded read of existing Application Signals service dependencies to the Fargate diagnosis worker, plus a deliberately inactive private CloudWatch Omni adapter whose activation is held pending a verified read-only per-space IAM boundary (Terraform rejects any nonempty diagnosis_omni_spaces). Both panels found the security posture sound — read-only, region-conditioned IAM, no ADR-005 impact, layered Omni hold — and neither found a gate-blocking correctness defect in the production path. The remaining findings are capacity/coverage limitations of the fail-closed design and several documentation-accuracy issues.

2. Issues per lens

L2 — Code correctness

MINOR (downgraded from one panel's MAJOR; chair-verified) — the managed logical-resource budget is bundle-global (20) while the docs imply per-region capacity (40 nodes/region), making the feature near-inert on large accounts. One model flagged this MAJOR; the other found no L2 defect. Verified against the diff: MAX_NEW_RESOURCES/MAX_MANAGED_RESOURCES count managed: resources across the whole bundle, and _resource's ID includes the region, so the same service in two regions consumes two of the 20 slots. In _signals, hitting the cap on a source service breaks and _fails the region's coverage, and _qualify then disqualifies every already-emitted row in that region. Consequence: an account with >20 instrumented services in one region yields zero fresh managed evidence there, and later regions cap out immediately (earlier complete regions do survive, per-region _qualify). I am not adopting this as MAJOR because it is the PR's explicitly intended qualification model, not an escaped bug: the new docs state "caps … disqualify earlier rows from the affected provider batch" and "20-slot share," the plan document records the slot arithmetic, and test_late_orphan_logical_cap_disqualifies_prior_calls asserts exactly this behavior. It fails closed (incomplete/disqualified, never wrong facts), consistent with the repo's evidence-integrity rule. The genuine residual defect is the doc phrasing (see L5) plus a practical-capacity concern worth a follow-up.

MINOR (one model) — enabling the flag unconditionally shrinks inventory anchors from 140 to 120 (_read_inventory slice minus _managed_resource_share()), even when Application Signals returns nothing, so accounts without instrumentation lose coverage for no gain.

MINOR (one model) — in the dependency loop, a cap-exhausted target of None is still passed to _endpoint_alias and _emit without a break, re-marking managed_discovery_resource_cap repeatedly for the rest of the page. Diagnostics noise only; no fact corruption.

MINOR (one model) — a single per-region coverage dict is shared between list_services and all list_service_dependencies paginations, so coverage["continuation"] is overwritten by the last paginator and can read false after an earlier truncated catalog. complete: false still fires via _fail, so disclosure stays conservative.

MINOR (one model) — _keys() rejects documented KeyAttributes shapes (e.g. rows carrying AwsAccountId, or AWS::Service without Environment), skipping them as application_signals_identity_unconfirmed. Fails closed and disclosed, but can render the collector inert on real accounts; validate against a live ListServices response before enabling.

Verified-correct notes shared across panels: the _resource share arithmetic sums exactly to MAX_RESOURCES (120+20+20+40); failed HTTP attempts count against MAX_CALLS before re-raising; the member path uses only the verified registered member session; the member_trace_network_sources_unsupported → member_xray_network_sources_unsupported rename has no dangling references.

L3 — Security / AWS mutation safety

No CRITICAL/MAJOR — both models agree. The only new IAM grant is application-signals:ListServices/ListServiceDependencies on Resource: "*" (these List actions have no resource-level support) with the aws:RequestedRegion condition from local.runtime_read_condition (chair-verified present in base workers.tf/workload.tf), gated on three flags and attached only to the worker task role. No mutation, instrumentation-enable, or service-activation action; no cloudwatch:GetSpaceCredentialsForOrganization grant anywhere; ADR-005 is untouched and no new owner-override is claimed. The Omni hold is enforced in four independent layers (Terraform validation, no env wiring, collect() calls only _signals, and a production test that pytest.fails — a BaseException not swallowed by _managed_collect's except Exception — if _omni is ever invoked). No hardcoded secrets; credential-shaped literals are test fixtures, and every failure branch asserts they never serialize into the bundle.

MINOR (one model) — the dead _space_client/_omni adapter still constructs real sessions from vended credentials; the only production guard is that nothing calls it. Credential handling inside it is sound (no caching, STS identity check, fixed error reasons), but a structural no-caller assertion would harden the hold against future refactors.

MINOR (one model) — evidence.py widening is slightly broader than needed: adding application_signals/cloudwatch_omni to _project's nested-mapping set also applies to facts, and several of the new FACT_KEYS entries (topology_complete, requests, the two source names) are redundant with coverage projection. Tighten so the fact allowlist stays as narrow as its comment claims.

L4 — Observability / data-integration correctness

No Steampipe/CIS surface is touched. Both models agree the provenance model is correct and well-enforced: provenance_family separates application telemetry from configuration, RUNS_ON/endpoint_alias are emitted only as configured_dependency, prompt rules in pillar_agents.py forbid double-counting Signals/Omni/X-Ray projections of the same telemetry and forbid promoting membership/aliases to instance calls, and managed evidence evicts first under the byte cap.

MINOR (one model) — Signals "fresh" means the bounded request's echoed window was confirmed, not that a call was observed recently within it (the API exposes no per-dependency timestamp, unlike Omni's lastObservedAt check). The asymmetry is inherent to the API but should be stated explicitly in diagnosis-discovery.md.

MINOR (one model, codex) — the documented "40 nodes" regional bound is not a unique-node counter across a region's dependency responses; _rows caps per response and native-bound targets bypass the managed-slot counter, so a region can involve more than 40 native dependency nodes before the 16-request or global bundle caps bite. This is the mirror image of the L2 capacity finding — the documented bounds don't match the enforced ones in either direction; align them once (see L5).

MINOR (one model) — _window's strict start <= left < right <= end rejects any provider-side rounding/clamping of the echoed window, a silent zero-evidence mode (fails closed as application_signals_window_unconfirmed); validate against a live response.

MINOR (one model) — _managed_collect's escaped-failure handler disqualifies both managed sources across all regions even for a single-region failure. Conservative and acceptable; deserves a comment noting it is intentionally wider than _qualify's per-region scope.

L5 — Docs / ADR consistency

README (EN+KO), CHANGELOG (EN+KO, correctly amending the existing evidence-loop bullet in place per the CLAUDE.md rule rather than appending), 06-workers.md, diagnosis-discovery.md, and the plan document are mutually consistent on the headline claims, and both cross-reference anchors resolve. The decision to ship no new ADR is sound: every grant is a read, and ADR-005 governs mutation autonomy.

MINOR (both models converged, different directions) — the "Bounds and evidence qualification" section's "per-region limit of 16 requests, 40 nodes" misstates the enforced limits: the 20-slot managed share is bundle-global (making 40 logical nodes per region unreachable), while native dependency nodes are not uniquely counted at all. Fix the wording to state the actual global 20-logical-slot ceiling and per-response row caps. This is the documentation half of the L2 capacity finding.

MINOR (one model, codex) — diagnosis-discovery.md says only native-to-native Omni CALLS edges can be fresh application calls, but _omni marks a traced recent edge fresh with resource_binding: "unresolved" when binding fails. Chair-verified mitigation: EvidenceIndex rejects unresolved bindings for observation claims, and the adapter is inactive, so there is no admission-path effect — align doc or code wording.

MINOR (one model) — 06-workers.md dropped the explicit "All grants are read-only" sentence in favor of "region-conditioned"; restore the read-only assertion alongside the new Omni disclaimer.

MINOR (one model) — managed_discovery_rejects_registered_member_space_activation duplicates the host rejection case (the validation is unconditional on member ARNs), so it adds no independent coverage; docs/guides/diagnosis-evaluation.md was not refreshed for the new provider (nothing there is now false).

3. Suggestions

1. In one change, reconcile the documented and enforced bounds: state that the 20 managed logical slots are bundle-global across regions, remove or qualify the "40 nodes per region" figure, and consider either raising the share, making it per-region, or releasing it when Signals returns nothing (which would also fix the unconditional 140→120 inventory shrink).
2. Add a structural guard that _omni/_space_client have no production caller (e.g. an import-graph or call-site assertion), supplementing the behavioral pytest.fail test.
3. Before enabling in a real account, validate _keys() and _window() against live ListServices/ListServiceDependencies responses — both are strict enough to silently zero out the collector (fail-closed, but inert).
4. Trim the redundant FACT_KEYS additions and scope the _project mapping widening to coverage, restore the "read-only" sentence in 06-workers.md, add the one-line comment on _managed_collect's intentionally global disqualification, and add a break when the dependency loop's target hits the resource cap.

4. Verdict

No CRITICAL finding from either model. The single panel MAJOR was verified against the diff and downgraded: the behavior it describes is the PR's explicitly documented, tested, fail-closed batch-disqualification design (the repo's required "capped evidence reads as incomplete, never healthy" posture), with the actionable residue being a docs-accuracy MINOR that both models independently touched from opposite directions. Security review is clean on both panels: read-only region-conditioned IAM, ADR-005 untouched, Omni credential vending verifiably unreachable in production. No required image evidence exists.

---

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