[Atom-oh]

Add scoped discovery and evidence admission, bounded DNS/network observation collectors, measured EC2 memory coverage, and an evidence-backed diagnosis loop across all six Well-Architected pillars. Validate model-selected findings through code-owned admission, bounded verification and explicit coverage gaps. Integrate the loop into worker reports and document evidence strength, account boundaries and model requirements.

The evidence and network gates default to off. Member reads require account registration and an exact role allowlist; this remains analysis-only under ADR-005.

Target dev so the existing automatic AI Code Review and required CI can evaluate the complete integration scope. The unmerged discovery foundation is also being reviewed in #246; its prerequisites and review findings must be resolved before integration. #244 was merged into the foundation branch; #242 is already merged into dev.

Validation:

• git diff --check passed for the current complete dev comparison at source HEAD 3b6aa45c003e8a69c50428f5c73c2a8002c57044.
• The earlier local unit result (201 passed) covered historical HEAD 02e141e41b6426247843c52c087ade7fded12a5e, not the updated integration scope.
• Fresh AI Code Review and required CI are requested for this dev scope. Integration remains pending their current-HEAD results; live AWS/model/runtime release checks have not been performed in this review-trigger operation.

[github-actions]

🤖 AI Code Review (two independent reviewers)

Reviewer responses: batch 1/7: codex/ALL claude/ALL; batch 2/7: codex/ALL claude/ALL; batch 3/7: codex/ALL claude/ALL; batch 4/7: codex/ALL claude/ALL; batch 5/7: codex/ALL claude/ALL; batch 6/7: codex/ALL claude/ALL; batch 7/7: codex/ALL claude/ALL

Status: BLOCKED — CRITICAL/MAJOR issues must be resolved before merge

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

Batch 1/7 — FAIL

All key claims are now verified against BASE. Synthesizing the chair review.

Chair Review — PR #245 "evidence-backed six-pillar diagnosis" (batch 1/7)

Summary

This batch adds the default-off diagnosis evidence-loop gates (two new Terraform flags, two ARN allowlists, two worker-task IAM policies), the CI plan-scanner exemption for the member-role ARNs, a thread-safe report budget ledger, handler wiring for budget/broken-connection handling, and redaction hardening. The ADR-005 frozen-mutation boundary holds — every new IAM action is a read or a bounded analysis-query verb, both panels agree, and I verified no mutation verb in the diff. The gate fails on one confirmed cross-model correctness MAJOR in handlers.py and two verified documentation-consistency MAJORs (onboarding runbook and README flag tables, neither updated in any batch of this PR).

L2 — Code correctness

MAJOR (both panels agreed; CONFIRMED against the diff) — scripts/v2/workers/handlers.py: the broken flag that routes the final finish_report through a fresh connection is computed too late and too narrowly. It is assigned from summary["degraded"] only immediately before the success-path _finish_report, after _upload_markdown and _export_artifacts. If the connection was broken during generation (the exact scenario _finish_report exists for) and _upload_markdown then raises, the except block sees broken == False and str(e) equal to the S3 error — not the exact sentinel "database_connection_broken" — so it writes status="failed" through the dead connection, which raises again, masks the original error, and leaves the pre-created diagnosis_reports row stuck in running (the UI polls forever). Compute broken immediately after rpt.generate() returns, and/or make _finish_report fall back to a fresh connection on any write failure rather than deciding in advance from a string comparison.

MINOR (one model flagged; verified) — ReportBudget(tier, ...) is constructed before wdb.connect() and outside every try, and raises ValueError for an unknown tier. BASE report.py:83-87 deliberately tolerates unknown tiers (TIER_CATALOG.get(tier, SECTIONS)), so a replayed/older queue payload that BASE handled now aborts with no finish_report write — another stuck-running row, but only when the loop is enabled. Normalize the tier or move construction inside the try.

MINOR (one model flagged; verified ordering) — summary["budget"] = budget.snapshot() runs before the rpt.make_title_and_tags fallback, which is an unbudgeted Bedrock call (BASE report.py), so the persisted ledger can understate real spend despite the comment claiming it covers "local metadata selection."

MINOR (one model flagged; partially verified) — budget.py hard-codes MAX_OUTPUT_TOKENS = 8192 while BASE report.py:175 honors DIAGNOSIS_MAX_OUTPUT_TOKENS (default 32000); the docstring claim "report.py imports these" is not true of BASE and is an unverified cross-batch dependency on the listed scripts/v2/workers/diagnosis/report.py. Whether reserve() callers ever pass a value above attempt_tokens() depends on the listed loop.py/pillar_agents.py (also unverified cross-batch).

MINOR (one model flagged; logic verified) — three tftest runs (diagnosis_member_roles_pin_the_onboarding_role_name and the diagnosis_member_role_arns halves of the two diagnosis_allowlists_reject_* runs) leave diagnosis_evidence_loop_enabled unset, so the "requires the evidence loop" validation already fails and expect_failures passes even if the role-name regex and host-account check were deleted. The Python test_ci_runtime_policy.py layer does cover those cases, so the rule isn't unguarded — but the Terraform tests don't test what their names claim. Add the enabling flags to those runs.

MINOR — _check_member_positions hard-requires after["role"], which is "known after apply" on a first combined apply (same pre-existing exposure as _check_target_policy); staged rollout avoids it, worth a comment. One model's claim that tier attempt/output ceilings are "mutually unreachable" did not hold up arithmetically (the review holdback is a single reservation, not per-attempt), so I did not adopt it.

Cross-batch dependencies (non-security, named): rpt.evidence_loop_enabled(), the budget= kwarg, summary["title_and_tags"]/summary["budget_incomplete"], and the "database_connection_broken" degraded sentinel are absent from BASE and expected from the listed report.py/loop.py/discovery.py; budget.py's tests are the listed test_budget.py. Plausibly provided; not treated as missing.

L3 — Security / AWS mutation safety

No CRITICAL — the ADR-005 frozen boundary holds (both panels agreed; chair verified). New grants attach only to aws_iam_role.worker_task[0], default off behind diagnosis_evidence_loop_enabled → workers_enabled and the separate network gate; the networkflowmonitor:Start/Stop*QueryMonitorTopContributors verbs create/cancel analysis query jobs only (no CreateMonitor/Update*/Delete*), the tftest asserts frozen gates stay off, and no secrets appear in the diff.

MINOR (one model flagged MAJOR; chair downgraded — the union is pre-existing BASE code, out of diff scope for the gate) — the new allowlist + ExternalId condition is not effective at the IAM layer whenever network_path_check_enabled or sg_rule_activity_enabled is on: I verified network-path.tf:51-55 and sg-rules.tf:176-180 already grant the same worker-task role sts:AssumeRole on arn:aws:iam::*:role/AWSopsReadOnlyRole with no condition (a documented repo convention also used by workload.tf:208). The new statement is strictly tighter than that precedent, which is the right direction; the in-diff defect is only that variables.tf's description and the tftest error message ("Member reads add exactly one sts:AssumeRole statement … with an external ID") overstate what IAM enforces on the role as a whole. Soften the wording or note that the runtime check lives in worker code.

Note carried to L5 as MAJOR — the mandatory StringLike: sts:ExternalId = "?*" condition denies member reads for 1st-party accounts onboarded without an ExternalId, which ADR-011 (amended 2026-06-26) explicitly supports (verified in web/lib/aws-assume.ts:2,33 and onboard-target-account.md:239-240). The condition itself is fail-closed and the variable description does say an external ID is required, so I treat the IAM design as intentional tightening rather than a security defect — but the unreconciled documentation is blocking (see L5).

MINOR (one model each; agreed non-blocking) — logs:GetQueryResults/StopQuery on * reproduces the existing ai_cost_logs_read precedent (availability-only exposure, no DescribeQueries to enumerate IDs); the aws:RequestedRegion condition resolves to all enabled regions (verified runtime-read-scope.tf:74-85, all_regions = true), so the three "limited to the runtime regions" claims are weaker than they read — pre-existing pattern, the tftest's 3-region list is a mock artifact.

redact.py — net improvement, three MINOR gaps (one model; chair verified (a) against the regex): (a) an IPv6 address followed by hex-ish text without a separator (2001:db8::1.ec2, or two addresses joined) forms one unparseable candidate that is emitted unredacted — _ipv6 only retries by trimming trailing ./:, not by shrinking the hex body; fails open on the LLM-privacy boundary. (b) a long run of :/. makes the retry loop O(n²) parses on up-to-96KB attacker-influenced collected text. (c) the hostname rule's region alternation misses ISO regions and *.on.aws endpoints even though the ARN rule now covers aws-iso* partitions.

L4 — Observability / data-integration correctness

No blocking finding (both panels agreed). The read_replica_source_db_instance_identifier column is a real aws_rds_db_instance Steampipe column persisted into JSONB — no migration or IAM needed; its consumer is an unverified cross-batch dependency on the listed discovery.py. cloudwatch:ListMetrics is the right minimal addition (BASE worker_diagnosis already grants GetMetricData). MINOR notes: the two Compute Optimizer actions duplicate worker_finops_baseline's grant (harmless, intentionally separately gated — worth a cross-reference comment); reserve() never refunds a crashed attempt and snapshot() correctly distinguishes known_usage from unknown_usage_attempts rather than silently summing — good design; the "associated with the selected VPC" DNS-log requirement is enforced only in worker code, which the variable description honestly states.

L5 — Docs / ADR consistency

MAJOR (one model flagged; chair verified — file changed in no batch) — docs/runbooks/onboard-target-account.md:231-240 says WorkerTaskRoleArn is needed only for "those two features" (sg-rules scan, Network Path Check) and that "1st-party (same-org) accounts can omit" the ExternalId. This PR adds a third worker-driven member job on the same role and makes an ExternalId mandatory for it at the IAM layer. An operator following the current runbook gets silently empty member evidence twice over (omitted WorkerTaskRoleArn, omitted ExternalId). The runbook appears in neither this batch nor the PR-wide path list; the listed docs/reference/diagnosis-discovery.md may document the requirement but cannot fix the contradicting onboarding instructions.

MAJOR (one model flagged; chair verified) — README.md's parallel EN/KO feature-flag tables (≈lines 155-161 / 394-400) document every other default-off gate (finops_baseline_enabled, sg_rule_activity_enabled, connector_target_sync_enabled, …) and gain no rows for diagnosis_evidence_loop_enabled, diagnosis_network_evidence_enabled, diagnosis_dns_log_group_arns, diagnosis_member_role_arns. README.md is in no batch of this PR and no CI test enforces the table.

MINOR (verified) — terraform/foundation/workers.tf:914-915 still claims "The diagnosis report job can run on either compute path," contradicted by handlers.py:325 ("report": (_report, "fargate")) and by this PR's own tftest assertion "the Lambda runs no diagnosis." The PR edits both files; fix the stale comment.

MINOR (unverified cross-batch) — the CHANGELOG sentence about DNS/NFM web caches and the offline synthetic benchmark describes code already in BASE (web/lib/nfm.ts BoundedTtlCache) with no web/ path in this batch or the PR-wide list — possibly a stacked-PR catch-up, otherwise it overstates this PR. Otherwise the CHANGELOG amendment follows the repo convention (amends the existing feature bullet in place, EN/KO 1:1, no PR/round numbers) — codex concurred.

Suggestions

1. In handlers.py, set broken immediately after rpt.generate() returns and make _finish_report retry on a fresh connection on any write exception; normalize unknown tiers before constructing ReportBudget; move budget.snapshot() below title selection.
2. Add workers_enabled = true + diagnosis_evidence_loop_enabled = true to the three member-ARN validation tftest runs so they exercise the regex/host-account checks they name.
3. Update onboard-target-account.md (third worker-driven job; ExternalId now required for diagnosis member evidence) and add the four new variables to both README flag tables — ideally in this PR.
4. In redact.py, shrink the IPv6 candidate from the right (not only trailing punctuation) and bound the retry window.
5. Align the diagnosis_member_role_arns description and tftest error message with what IAM actually enforces given the pre-existing wildcard AssumeRole grants, or scope those grants in a follow-up.

Verdict

One confirmed cross-model L2 MAJOR (handlers.py broken-connection ordering) plus two verified L5 MAJORs (onboarding runbook and README, neither touched by any batch) block this batch. No CRITICAL; the ADR-005 read-only boundary is intact.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 2/7 — PASS

All load-bearing panel claims are now verified against BASE. Synthesizing the chair review.

Chair Review — PR #245, batch 2/7 (scripts/v2/workers/diagnosis/discovery.py)

Summary

This batch adds a single new 1,313-line module: code-owned, bounded evidence discovery for the six-pillar diagnosis worker (inventory/graph SELECTs, X-Ray, CloudWatch, Compute Optimizer, FinOps reads). Both panels independently confirm the read-only guarantee holds — every AWS call is a Get* read, every SQL statement is a SELECT/session-timeout setting, and the host/member scope gate fails closed in both directions — so no ADR-005 issue exists in this batch. The gate-relevant panel MAJORs all resolve, on verification, to either unverified cross-batch dependencies on files explicitly listed in other batches (which review them independently) or to a defensive-path defect with no concrete failure trigger in the resulting code; what remains in-batch is a set of MINOR robustness and disclosure issues.

L2 — Code correctness

No gating issue. Claude's deep reconciliation (schema + migrations, sync writer, TS parity sources) checked out; codex found no batch-local correctness defect — models agree.

• Unverified cross-batch dependency (claude flagged MAJOR; chair-verified at BASE) — read_replica_source_db_instance_identifier is never selected by BASE scripts/v2/steampipe/sync_lambda.py (confirmed: zero matches), so against BASE alone _replica_source would always return "unknown" for non-Aurora RDS and no provider recommendation could clear replica_source_unknown. scripts/v2/steampipe/sync_lambda.py is listed as changed in another batch and plausibly adds the column (the _facts comment "a synced null means standalone" presupposes it). Not gating here per the cross-batch rule; the batch reviewing sync_lambda.py must confirm the column is projected, with explicit null preserved distinctly from absence.
• MINOR (one model — claude; chair-verified) — _endpoint claims exact parity with trace-graph.ts exactEndpoint but is weaker: TS rejects hosts >253 chars and invalid labels and strips exactly one trailing dot (replace(/\.$/,'')); Python rstrip(".") strips all trailing dots and validates nothing. The comparison against raw inventory endpoint_address runs the weaker form, so the docstring overstates the guarantee.
• MINOR (one model — claude) — _arn hardcodes the aws partition in synthesized ARNs while _REGION admits GovCloud regions; in us-gov-* the wrong-partition ARNs surface only as one blanket provider_object_errors marker.
• MINOR (one model — claude) — _provider_row's row.get("tags", []) returns None for an explicit null (unlike the file's own or [] pattern two lines up), and the resulting TypeError escapes to _provider_sizing's outer except, abandoning the whole region/kind collection as provider_sizing_read_failed.
• MINOR (one model — claude) — _client's TimeoutError("discovery_deadline") is raised inside the try blocks of _xray/_metrics/_provider_sizing, so budget exhaustion is misreported as *_read_failed instead of discovery_deadline.
• MINOR (one model — claude) — when db_instance_identifier fails the _IDENTITY_KEYS shape check, _facts stores the redacted/truncated text and _metrics accepts it as a CloudWatch dimension (its local guard checks only type/:/length), so the inevitable empty series is scored as coverage loss rather than metric_identity_unresolved.
• MINOR (one model — claude) — MAX_BYTES is a soft bound: a bundle whose non-shrinkable coverage/native_sources exceed the budget is returned oversized after the eviction loop breaks; and the inner observation_count > MAX_RELATIONSHIPS break only exits the per-edge pair loop (wasteful, not wrong).

L3 — Security / AWS mutation safety

Both models agree the read-only guarantee holds: SDK surface is exactly xray:GetServiceGraph, cloudwatch:GetMetricData, compute-optimizer:Get{EC2Instance,RDSDatabase}Recommendations; DB surface is SELECT-only plus session statement_timeout management; no mutation, invoke, shell, or hardcoded secret. The module-level client cache only ever holds host-role clients, member paths always use the explicit member_factory, and collect/follow_up reject scope/host mismatches fail-closed. The feature is double-gated on DIAGNOSIS_EVIDENCE_LOOP_ENABLED. No ADR-005 violation.

• Unverified cross-batch dependency (claude flagged MAJOR; chair-verified at BASE) — the inline IAM citations (worker_diagnosis_evidence / Sid DiagnosisProviderSizingReadOnly with runtime_read_condition) do not exist in BASE terraform/foundation/workers.tf: the only Compute Optimizer grant there is FinopsBaselineReadOnly on the FinOps role, carrying no Condition (workers.tf:894-912); cloudwatch:GetMetricData and xray:GetServiceGraph are satisfied by the existing DiagnosisDataSourcesReadOnly Sids (workers.tf:861-874, 931-944). This batch introduces no IAM change of its own, and a missing grant fails closed (AccessDenied → provider_sizing_read_failed degraded marker), so no least-privilege violation is present in this batch's code. terraform/foundation/workers.tf and tests/runtime_iam.tftest.hcl are listed in other batches (all terraform is batch 1, which gates IAM with the same checklists); batch 1 must confirm the new role/Sid exists exactly as cited — two actions only, runtime_read_condition attached — or this comment is the file's only IAM record and is wrong.
• Unverified cross-batch dependency (claude flagged MAJOR; codex concurred as cross-batch) — the entire raw-facts design (unredacted scope-matched ARNs, private_ip_address, endpoint_address, db_host, instance ids) defers masking to evidence.EvidenceIndex.model_context(redacted=True) in scripts/v2/workers/diagnosis/evidence.py (other batch). Chair-verified: BASE redact() masks only arn:aws: ARNs, account ids, emails, IPs and AKIDs — it does not mask hostnames, so the docstring's "masks host fields" must be additional behavior implemented in evidence.py; a plain redact() pass would send db_host/endpoint_address verbatim to Bedrock. Nothing in this batch sends data to a model, so no leak exists here, but the batch reviewing evidence.py/redact.py must verify host masking exists, or this is a blocking data-exposure finding there.
• MINOR (one model — claude; chair-verified against redact.py) — REDACTORS matches only arn:aws:…; out-of-scope arn:aws-cn:/arn:aws-us-gov: ARNs that fall through _facts to _text() keep partition/service/region/resource path (only the embedded account id is scrubbed).
• MINOR (one model — claude) — the edges query ships whole instance_observations pair objects unprojected through pg8000, contradicting the module's own "projection happens inside SQL" statement and the agent-sql-reader "never list a JSONB blob raw" rule; Python reads only known keys afterward, so exposure is bounded.
• MINOR (one model — claude) — a failed statement_timeout restore leaves the caller's session with this module's timeout, disclosed only via database_timeout_restore_failed.

L4 — Observability / data-integration correctness

No Steampipe query or CIS check changes in this batch. The CloudWatch batching math (≤96 query ids, ≤32,256 datapoints vs MaxDatapoints=100800), page/token bounding, _REQUIRED_METRICS not-published-vs-loss split, X-Ray rate math, and _inventory_source_fresh parity with inventorySourcesStale all verified clean by claude.

• MINOR (codex flagged MAJOR; chair downgraded — no concrete failure trigger in resulting code) — in _facts, a present non-string read_replica_source_db_instance_identifier (number/bool/object, not just JSON null) becomes "", which _replica_source reads as "none" (standalone), suppressing ReplicaLag collection and the replica_source_unknown guard. Verified real in the diff. However, the only writer of that field is the trusted sync projecting an RDS API string-or-null value; no code path in the PR produces a malformed non-string, so the fail-open is a defense-in-depth gap against hypothetical data corruption, not a reachable defect. Claude did not flag it. Fix is one line: treat only None as standalone ("" if value is None else mark unknown). Should be fixed, but does not gate.
• MINOR (one model — codex) — _provider_sizing never records requested ARNs absent from a successful, error-free Compute Optimizer response, so a partial response is indistinguishable from complete coverage; track returned ARNs and mark unmatched resources incomplete.
• MINOR (one model — claude; chair-verified) — coverage.graph[cls].instance_observations_omitted is derived from instanceObservationDrops, which the TS side deliberately never stores (three BASE tests assert its absence), so the class-level field is a hardcoded 0 presented as a positive "nothing omitted" claim next to per-edge counters that can be positive. Real omissions are still caught per edge, so nothing is hidden — but drop or sum the field.
• MINOR (one model — claude) — jsonb_agg(pair) over an ORDER BY n subquery does not guarantee aggregate order in PostgreSQL; use jsonb_agg(pair ORDER BY n) to make the "stored order / 201st sentinel" comment true. Practically unreachable since the writer caps at 200 (chair-verified INSTANCE_OBSERVATION_CAP = 200).
• MINOR (one model — claude) — _graph_state bounds GRAPH_REBUILD_INTERVAL_MINS at 1440 while readGraphState accepts unbounded values; Python falls back to the stricter 15-minute floor above 1440 (fails closed, but the undocumented parity divergence deserves a note).

L5 — Docs/ADR consistency

No docs or ADR body changes in this batch (both models agree). The repo carries only ADR-023 in-tree; the ADR-005 boundary was verified against implementation (L3) and holds.

• MINOR (one model — claude) — the file names its own single most important contract two different ways: the module docstring says evidence.EvidenceIndex.model_context(redacted=True), the _IDENTITY_KEYS comment says evidence.redact_context. These must agree and match what evidence.py (other batch) actually exports.
• MINOR / unverified cross-batch — all documentation for the new coverage vocabulary (_REASONS, incomplete/degraded/gaps markers), caps, and the follow_up capability set lives in listed other-batch files (docs/reference/diagnosis-discovery.md, docs/reference/06-workers.md, docs/guides/diagnosis-evaluation.md, CHANGELOG.md); the batches containing them must confirm the marker vocabulary and follow-up contract are documented.
• MINOR (one model — claude; chair-verified) — the two IAM comment citations point at a role/Sid that does not exist at BASE (see L3); they are consistent only if batch 1's workers.tf introduces them exactly as named.

Suggestions

1. Fix the _facts replica-source coercion so only explicit JSON null means standalone; non-string malformed values should stay "unknown" and mark coverage incomplete.
2. Normalize row.get("tags") or [] (and the estimatedMonthlySavings dict access) so one malformed provider row cannot abort a region/kind.
3. Catch TimeoutError("discovery_deadline") separately from source failures in _xray/_metrics/_provider_sizing so budget exhaustion is disclosed as discovery_deadline.
4. Use jsonb_agg(pair ORDER BY n); re-check _IDENTITY_KEYS at the _metrics dimension site; either populate or drop the class-level instance_observations_omitted; align the two redaction-boundary names.
5. For batch 1's chair: verify worker_diagnosis_evidence/DiagnosisProviderSizingReadOnly exists with exactly the two Compute Optimizer actions and runtime_read_condition, and that the updated redact/evidence.py actually masks host fields before any Bedrock call.

Verdict

No CRITICAL/MAJOR finding survives verification within this batch's diff: the read-only/scope-isolation guarantees hold, the panel MAJORs are either dependencies on files this PR changes in other, independently gated batches (IAM in batch 1 fails closed if absent; redaction boundary in the evidence.py batch; replica column in the sync_lambda.py batch) or lack a concrete failure trigger in the resulting code (the malformed-replica-value coercion, downgraded to MINOR). The image manifest lists no images and no unavailable/omitted entries; both panels and the chair concur.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 3/7 — FAIL

I've verified the load-bearing claims: BASE redact.py confirms both redaction gaps (ARN rule is arn:aws: partition-literal, IP rule is IPv4-only), the diff confirms the multi-region DNS truncation, and codex's NetworkFlowMonitor limit-parameter claim is a false positive (the StartQueryMonitorTopContributors API accepts limit; maxResults is the separate results-pagination knob). Final synthesized review follows.

---

Chair Synthesis — PR #245, Batch 3/7 (scripts/v2/workers/diagnosis/{evidence,loop,memory_metrics,observations}.py)

1. Summary

This batch adds the evidence-admission core (evidence.py), the bounded analysis loop (loop.py), and two read-only collectors (memory_metrics.py, observations.py) for the six-pillar diagnosis. The overall architecture is strongly fail-closed and ADR-005-clean (reads and own-query cancellation only, double default-off env gating, host-account scoping), but the batch carries one verified correctness MAJOR (silent multi-region DNS evidence truncation) and verified redaction gaps (non-standard-partition ARNs and IPv6 addresses reach the model unmasked) plus new IAM grants that can only be adjudicated in batch 1. Base reconciliation: discovery, budget, pillar_agents, scoped_clients, workload_identities and the modified redact.py are all in the PR-wide changed-path list — treated as unverified cross-batch dependencies, not missing symbols; from redact import redact resolves against BASE scripts/v2/workers/redact.py.

2. Issues per lens

L2 — Code correctness

MAJOR — observations.project silently discards DNS rows beyond one region's worth (claude flagged MAJOR; codex did not flag). Verified in the diff. collect accumulates dns_rows across up to MAX_REGIONS = 8 regions, each _dns call returning up to MAX_GROUPS × MAX_ROWS = 400 rows, but project iterates dns_rows[:MAX_ROWS * MAX_GROUPS] — a 400-row cap with no d._mark(..., "incomplete", ...). The flow-side slice (MAX_ROWS * MAX_QUERIES = 300) exactly matches its producer's global bound, which confirms the DNS slice is an unintended single-region assumption rather than a deliberate cap. Trigger: ≥2 regions each returning a full Resolver result set — observed dependency pairs from later regions vanish with no disclosure, violating this module's own every-cap-is-marked discipline.

MINOR — accumulated GetMetricData statuses reject any paginated read (claude flagged MAJOR; chair downgrades). memory_metrics.collect appends StatusCode per page and requires all to be "Complete"; CloudWatch returns PartialData on non-final pages, so any paginated response yields reason="memory_metric_samples_incomplete". However, MaxDatapoints = EXPECTED × len(queries) structurally covers the maximum possible datapoint count (336 buckets per query at period 3600 over exactly 14 days), so pagination should not occur in practice, and when it does the code fails closed (no invented data). Latent fragility — require only the last status per Id to be Complete — not a live MAJOR.

MINOR — reviewer rerun directives can be silently dropped (codex flagged MAJOR; chair downgrades, claude did not flag). In loop.run, rerun_pillars is only honored inside if requests:; a review requesting reruns without follow-up collection is reported completed with no rerun and no marker, and _validate_review silently truncates pillars[:MAX_RERUNS]. Fail-closed for findings (the review's keep/reject set is still applied), and a rerun without new evidence is arguably excluded by design — but both drops deserve an incomplete marker for symmetry with follow_up:no_targeted_pillars.

MINOR — _fingerprint no-progress detection is defeated by collection metadata (codex only). Evidence identity retains window (fresh start/end timestamps) and source_id-bearing facts, so a refresh with byte-identical measurements still fingerprints as progress and spends the second billable pass.

MINOR (claude, verified plausible from diff): a cooperative review asking a 5th valid follow-up is discarded wholesale (len(requests) > MAX_FOLLOW_UP_REQUESTS returns None) instead of being truncated with a marker; rejected finding ids are not added to _finding_ids, so a rejected id can be resubmitted without tripping duplicate_finding_id; validate_request restricts to {"ec2", "rds"} while _sizing_supported accepts rds_instance (whether discovery emits that kind is a cross-batch question); project()/_network_matches index bundle["coverage"]["account"] unguarded while _qualified uses .get; len(contributors) is evaluated before the isinstance list check protects it; bare ValueError in _SOURCE_ERRORS misclassifies the module's own dns_query_vpc_invalid/invalid_source_rows defects as source gaps, contradicting the module header; the MAX_POLLS comment understates worst-case backoff (~9.25 s, and the final pause runs after the last poll); the index = next_index rebind window in loop.run can return pass-1 findings against a pass-2 evidence_bundle on an unexpected exception.

L3 — Security / AWS mutation safety

Read-only guarantee (ADR-005): intact — both models agree. All eleven AWS actions are reads or control-plane cancellation of the worker's own ephemeral Insights/NFM queries; nothing creates, updates or deletes a resource, nothing touches the frozen remediation substrate or claims a third owner-override. Gating is double default-off (DIAGNOSIS_EVIDENCE_LOOP_ENABLED and DIAGNOSIS_NETWORK_EVIDENCE_ENABLED), network evidence requires scope == "self" plus an exact DIAGNOSIS_HOST_ACCOUNT_ID match (fail-closed when unset), the Logs Insights query string interpolates only _VPC_ID-validated literals, and _groups rejects the whole allowlist on any malformed or foreign-account ARN. No hardcoded secrets.

MAJOR — non-standard-partition ARNs leak to the model (claude only; chair-verified against BASE). redact.py:14 masks only arn:aws:[^\s"']+, while this batch's region validators ([a-z]{2}(?:-gov)?-[a-z]+-\d|global in _admit_resources/_admit_evidence) deliberately admit us-gov-* and also match cn-* regions, and the ARN scope check never inspects the partition segment. A GovCloud/China resource_id reaches model_context(redacted=True) with partition, region, service and resource name intact (only the 12-digit account is caught by the standalone-account rule) — violating the module's own stated redaction contract and BASE redact.py's "mandatory scrub before ANY Bedrock call." redact.py is modified in another batch and may fix this; per the cross-batch rule a security finding stays blocking until confirmed. Fix: arn:aws[a-z-]*: in redact.py, or key-mask resource_id like _HOST_KEYS.

MAJOR — IPv6 addresses are never redacted (claude only; chair-verified against BASE). redact.py:19's only IP rule is IPv4; _HOSTNAME requires dots; _AWS_HOST doesn't match bare addresses. This batch's _answers explicitly admits AAAA records ((":" in address) != (kind == "AAAA")) and project writes answer_ips into evidence facts, so dual-stack resolutions send raw IPv6 addresses to the model. Same cross-batch caveat (redact.py), same blocking treatment.

MAJOR — new IAM grants are an unverified cross-batch dependency (claude only). The in-code provenance comments cite workers.tf Sids (DiagnosisMeasuredMemoryReadOnly, DiagnosisObservationDiscovery, DiagnosisApprovedResolverLogRead, DiagnosisQueryResultsAndCancellation, DiagnosisMonitorObservationQuery) that do not exist in BASE workers.tf (which has only worker_diagnosis/DiagnosisDataSourcesReadOnly — the one comment checkable against BASE, for GetMetricData, is accurate). terraform/foundation/workers.tf and runtime_iam.tftest.hcl are batch-1 files; per the cross-batch rule, IAM-scoping findings do not get severity relief from batching. Batch 1 must confirm: logs:StartQuery resource-scoped to the allowlisted log groups (the in-code _groups check is defense-in-depth, not an IAM boundary), cloudwatch:ListMetrics carrying runtime_read_condition, no NFM write actions, and that both cited policies attach to the same worker role.

MINOR (claude): finding ids pass through redact_context byte-for-byte but are validated by the permissive _ID (digits/dots/colons allowed), making the "no ARN/account id can be one" comment true only of _OPAQUE_ID; tag values (1024-char free text) survive into model context — the <untrusted> wrapping lives in pillar_agents.py (other batch); collect_proxies is the only source call outside an error guard; 1024-char truncation can defeat whole-string host masking for non-_HOST_KEYS keys.

L4 — Observability / data-integration correctness

No Steampipe or CIS surface appears in this batch; the substance is diagnosis admission logic, whose core invariants both models examined and which hold on chair re-verification: _OPAQUE_ID + duplicate counting makes citations code-owned; _native_identity canonicalization rejects double-entry (ambiguous_identity rejects both rows); _complete separates freshness from completeness; DNS-correlated edges always carry complete: False + binding: "dns_window_correlation" and therefore can never satisfy _observation_supported — the docstring's "DNS window matches remain hypotheses" is enforced twice.

REJECTED — codex MAJOR: "start_query_monitor_top_contributors does not accept limit". False positive. The NetworkFlowMonitor StartQueryMonitorTopContributors API accepts limit as an optional request member (the count of top contributors); maxResults/nextToken belong to GetQueryResultsMonitorTopContributors, which the code also uses correctly. Botocore would not reject this request; no degradation occurs. Excluded from the gate.

MINOR — any complete metrics row validates any single-resource "observation" claim (codex flagged MAJOR; chair downgrades, claude examined the same function without flagging). _observation_supported confirms fresh, complete, resource-scoped measured data exists, but cannot verify that the cited metric semantically supports the free-text assertion (e.g., an encryption claim citing CPU metrics keeps observation status). Bounded impact: explanations cannot carry numeric assertions, citations are real and resource-scoped, and the consequence is a presentation-tier label rather than invented data. Worth tightening (e.g., restrict observation to claims whose predicate is code-checkable, as _sizing_supported already does), but not blocking.

MINOR (claude): unit: "Bytes" is asserted into evidence facts when NFM omits unit — an unverified assumption recorded as measured fact in a module premised on never inventing values; flow_matched = True is set before the unit check, suppressing nfm_rows_unmatched; _network_matches' symmetric local/remote VPC+region requirement may be unsatisfiable for non-EC2 remote endpoints depending on real contributor payload shape (validate against a live GetQueryResultsMonitorTopContributors fixture); early returns in collect skip d._finish; a pillar dropped for finding_limit is marked degraded but not incomplete, breaking the file's otherwise consistent pairing; the budget.remaining() < 130 threshold is the only unnamed magic bound.

L5 — Docs / ADR consistency

Both models agree there is no ADR conflict: only read/query actions behind default-off flags, no third owner-override claimed, ADR-019 correctly not cited as precedent, and the house style of per-call IAM Sid provenance comments is followed (the one BASE-checkable comment is accurate).

MINOR (claude; codex deferred to other batches): all documentation for these four modules (docs/reference/06-workers.md, diagnosis-discovery.md, CHANGELOG.md, etc.) lives in other batches and must be confirmed there — unverified cross-batch dependency, non-blocking per the cross-batch rule. No listed runbook covers the new operator prerequisites (DIAGNOSIS_DNS_LOG_GROUP_ARNS allowlist, NFM monitor requirement, the three new env flags) or the ~30 new gaps/incomplete reason codes; docs/runbooks/steampipe-quota-and-staleness.md is the repo's precedent for tabulating such codes. Two different roles are cited for two calls made by one client in memory_metrics.collect — plausible (both policies can attach to one role) but batch 1 must confirm the attachment.

3. Suggestions

1. Slice dns_rows[:MAX_ROWS * MAX_GROUPS * MAX_REGIONS] (or mark incomplete whenever the slice elides rows) in observations.project.
2. Broaden the ARN redactor to arn:aws[a-z-]*: and add an IPv6 rule in redact.py (confirm whether the other-batch diff already does), or mask resource_id/answer_ips by key in _scrub_text.
3. In memory_metrics, require only the final per-Id status to be Complete and give MaxDatapoints headroom.
4. Add incomplete markers for dropped/truncated rerun_pillars and truncate over-limit follow-up lists instead of voiding the review.
5. Exclude window/source_id from _fingerprint identity so the no-progress check works for metric refreshes.
6. Record NFM unit only when the source supplied it, and move the unit check before flow_matched = True.
7. Batch 1: verify the five new IAM Sids exactly match the eleven actions called here, with runtime_read_condition on ListMetrics and log-group-scoped StartQuery; a later batch: document the env-flag/allowlist prerequisites and reason-code vocabulary.

4. Verdict

One verified correctness MAJOR (silent multi-region DNS evidence truncation), two verified redaction-contract MAJORs (partition-literal ARN rule and IPv4-only IP rule in BASE redact.py, with this batch admitting gov/cn regions and AAAA answers), and one blocking unverified cross-batch IAM dependency remain. Codex's two L4 MAJORs did not survive verification (one API false positive, one downgraded by design-context). Image manifest lists no images, no deletions, no unavailable or omitted entries; this batch touches no image file.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 4/7 — FAIL

All verification complete. Here is the synthesized chair review.

---

Chair Review — PR #245, batch 4/7 (evidence-backed six-pillar diagnosis: pillar agents, presentation, report orchestration, scoped member clients, supporting summaries)

1. Summary

This batch adds the gated evidence-loop diagnosis path: six structured Bedrock pillar calls plus a cross-pillar reviewer (pillar_agents.py), deterministic presentation (presentation.py), the _generate_evidence orchestrator in report.py, member-scoped STS clients (scoped_clients.py), and bounded account summaries (supporting.py). The security posture is largely strong (read-only AWS calls, prompt-injection hardening, mutating-CLI withholding, no secret leakage), but the batch carries one blocking unverified cross-batch IAM dependency for the new sts:AssumeRole path and two verified MAJOR correctness/integration defects (a model_runtime provider-shim bypass and a silent-success path when all supporting evidence is skipped). Claude's panel report produced the substantive findings; codex found essentially nothing beyond one valid retry MINOR — I verified claude's MAJOR claims directly against BASE rather than relying on single-model assertion.

2. Issues per lens

L3 — Security / AWS mutation safety

• MAJOR (BLOCKING, unverified cross-batch IAM dependency) — scoped_clients.py performs sts:AssumeRole into registered member roles, gated by the DIAGNOSIS_MEMBER_ROLE_ARNS env allowlist and a comment citing workers.tf worker_diagnosis_evidence Sid DiagnosisRegisteredMemberReadOnly scoped to diagnosis_member_role_arns. Chair-verified: neither that Sid, nor the variable, nor any DIAGNOSIS_MEMBER_ROLE_ARNS/DIAGNOSIS_HOST_ACCOUNT_ID/DIAGNOSIS_EVIDENCE_LOOP_ENABLED wiring exists anywhere in BASE terraform/foundation/. terraform/foundation/workers.tf, variables.tf, and tests/runtime_iam.tftest.hcl are listed in other batches and must supply it: the statement must be resource-scoped to exactly the configured member role ARNs (not arn:aws:iam::*:role/*), the env allowlist must be populated from the same Terraform variable, and the tftest must assert it. Per the cross-batch rule, IAM findings are never downgraded by batching; this stays blocking until the batch containing those files confirms it. One model flagged (claude); codex reported "no security finding" — a disagreement the BASE evidence resolves in claude's favor. The in-code controls that are present (12-digit account regex, enabled AND NOT is_host registry predicate, role-name charset check, ≤32-entry ARN allowlist, post-assume AssumedRoleUser.Arn prefix + GetCallerIdentity account verification) are good defense-in-depth.
• MINOR — the member read-only guarantee rests entirely on the member-side AWSopsReadOnlyRole; scoped_clients.client(service, region, budget) is a generic factory with no host-side service allowlist, unlike BASE's fixed-API callers (sg_rule_scan.py). A cheap service allowlist or test contract would close the gap (claude only).
• MINOR — the statement_timeout restore in scoped_clients.member_factory's finally can raise and shadow the in-flight ScopeError/DB error (claude only).
• MINOR — presentation.withheld constrains id to a charset but passes reason[:64] through uncharacterized into summary["rejected_findings"]; constrain to a code-owned enum to keep the "code-owned only" docstring claim true (claude only).
• No ADR-005 violation. Chair-verified: every AWS call added is read-only; the five data-source actions are already granted in BASE workers.tf:861-874 (Sid DiagnosisDataSourcesReadOnly, runtime_read_condition); _VERIFY templates use read-only verbs; model-authored mutating CLI is withheld via BASE's _has_mutating_cli/_MUTATING_MARK; no secrets are logged or returned. Both models agree on this. Prompt-injection posture (untrusted-data framing, _unique_object, _validate_shape, bounded response reads, redact_context, _cell escaping) is solid.

L4 — Observability / data-integration correctness

• MAJOR (verified) — pillar_agents._client builds a raw bedrock-runtime client, bypassing the model_runtime.wrap shim that every other Bedrock caller in the repo uses (chair-verified: report.py:226, finops/llm.py:42, insight/generate.py:36, graph_querygen.py:92, signal_catalog_gen.py:106). Chair-verified failure mechanism: under model_provider = anthropic_api, models.tf gives bare claude-sonnet-5-5 IDs (rejected by MODEL_CAPABILITIES as unsupported_model) and workers.tf:848 emits no BedrockInvokeReadOnly grant (runtime_model_resources empty); under bedrock_openai the model family is likewise unsupported. Enabling the evidence loop on such a deployment makes every analysis fail, collapsed by _generate_evidence's except Exception into a generic analysis_failed degraded report with findings: []. Either route through the shim or validate model_runtime.provider() up front with a distinct model_provider_unsupported marker. One model flagged (claude); the cross-batch docs/runbooks/model-provider.md may document a Claude-only constraint, but the convention break and indistinguishable failure marker are in-batch defects.
• MAJOR (verified in-batch; cross-batch caveat) — when DIAGNOSIS_HOST_ACCOUNT_ID is unset or mismatched, supporting.collect skips all four supporting sources (cost, posture, what-changed, commitment) but marks only gaps (supporting_host_unknown), never degraded. BASE handlers.py:105 computes status = "partial" if summary.get("degraded") or summary.get("render_degraded") else "succeeded", so the report claims success with an entire evidence tier silently absent — inconsistent with this batch's own siblings (discovery_failed and stage failures mark both gaps and degraded). Compounded by DIAGNOSIS_HOST_ACCOUNT_ID being a new, unwired env var (BASE only sets HOST_ACCOUNT_ID gated on datasource_diagnosis_enabled, workers.tf:45-54); wiring is an unverified cross-batch dependency on terraform/foundation/workers.tf, and handlers.py (also changed in another batch) may or may not alter the status rule. Add supporting_host_unknown to degraded in-batch regardless. One model flagged (claude); codex explicitly found no L4 issue — the BASE handler line confirms claude's mechanism.
• MINOR — the deterministic invariant bridge fails closed as intended (chair-verified: invariants._edges requires e["to"], the bridge emits to_ref — matching the documented live-integration limit at invariants.py:12 — and partial: True defeats _complete), but partial is the single guard; also pass degraded=True to src._result (claude only).
• MINOR — _cost's partial_month: False meaning "previous full month" invites misreading; _bounded_body's evidence pruning drops rows that also cite retained resources (disclosed via omitted_evidence, but needlessly lossy) (claude only).
• MINOR (cross-batch) — deep-tier Opus attempt-token arithmetic (six specialists + reviews + final-review holdback vs. the ceiling) is unverifiable here; budget.py/loop.py are in other batches (claude only).
• Verified-good and worth preserving: exclusive-End month handling in _cost, LINKED_ACCOUNT filtering, resource_cost: False, _posture's sort-fallback error taxonomy and not-subscribed steady state, repeated-token/page-cap guards, and _single_region disclosure (both models concur on this positive assessment).

L2 — Code correctness

• MINOR (downgraded from claude's MAJOR) — supporting._mark/collect index bundle["gaps"]/["incomplete"]/["evidence"] directly while report._generate_evidence defensively uses setdefault for the same bundle. A KeyError requires discovery.collect (another batch) to return a bundle missing those keys — no concrete in-batch trigger exists, and this batch's own fallback bundle sets all keys. Unverified cross-batch dependency on scripts/v2/workers/diagnosis/discovery.py; fix is cheap (setdefault in _mark) and worthwhile because the enclosing except Exception would mislabel a KeyError as cost_read_failed.
• MINOR (downgraded from claude's MAJOR) — conftest.py's autouse cache-clearing fixture (which exists precisely to stop cross-test client leakage) was not extended to the two new module-global caches pillar_agents._clients and supporting._clients. Test files for the new modules live in other batches and may compensate; test hygiene, not a product defect.
• MINOR — pillar_agents._structured retries ConnectTimeoutError/EndpointConnectionError but a botocore ReadTimeoutError raised during invoke_model/stream read falls into the generic handler and aborts without using the second attempt, defeating MAX_MODEL_ATTEMPTS = 2 for the most common transient (codex only; mechanism checks out against the except chain).
• MINOR — presentation.render keys routing on id(f); a duplicated finding object renders its full body twice, contradicting the "exactly once" docstring — key on the schema-required f["id"] (claude only).
• MINOR — assorted smaller edges (claude only): dead Config local in member_factory; unquantized (region, read_timeout) Bedrock client cache key (BASE deliberately quantizes, report.py _CALL_TIMEOUT_STEP_S); _posture returned counts unfiltered rows; None commitment percentage renders as literal None; prerequisites not capped at 4 in _finding_body and a numeral-containing non-mutating prerequisite is dropped with no marker; verification commands surface only for top-12 recommendations references; test_budget.py monkeypatches the global time module and its subprocess has check=True but no timeout.
• Chair-verified: all in-batch imports from BASE resolve (_thinking_fields, _MUTATING_MARK, _has_mutating_cli, _render_intended_vs_actual, localized_title, INTENDED_VS_ACTUAL_SECTION, build_markdown, src._result, ddb.list_active_invariants/get_report_summary); .budget/.evidence/.discovery/.loop/.observations are all listed cross-batch files.

L5 — Docs / ADR consistency

No doc files in this batch; all relevant docs (docs/reference/06-workers.md, diagnosis-discovery.md, model-provider.md, diagnosis-evaluation.md, CHANGELOG.md) are listed cross-batch dependencies. Both models agree nothing here implicates ADR-005 or the two owner-override carve-outs.

• MINOR — report.py's module and generate() docstrings describe only the legacy path; neither mentions the evidence_loop_enabled() short-circuit nor the new budget parameter (claude only).
• MINOR — three new env vars (DIAGNOSIS_EVIDENCE_LOOP_ENABLED, DIAGNOSIS_MEMBER_ROLE_ARNS, DIAGNOSIS_HOST_ACCOUNT_ID — a fifth spelling of the host-account var in this repo) have no in-batch documentation; docs/reference/06-workers.md (other batch) is the right home (claude only).
• MINOR — the scoped_clients.py inline IAM citation references Terraform that does not exist in BASE (see L3); the supporting.py IAM citations and the migration 01KW39F7... ExternalId citation are accurate (chair spot-checked the data-source Sid) (claude only).
• MINOR (cross-batch) — BASE handlers.py:101 still calls rpt.make_title_and_tags(md, lang), duplicating the deterministic summary["title_and_tags"] with an LLM call; and the deliberate RuntimeError("database_connection_broken") leaves the row for the reaper — both need the cross-batch handlers.py/runbook changes to land (claude only).

3. Suggestions

1. Land (and assert in runtime_iam.tftest.hcl) the DiagnosisRegisteredMemberReadOnly statement scoped to exactly diagnosis_member_role_arns, with DIAGNOSIS_MEMBER_ROLE_ARNS/DIAGNOSIS_HOST_ACCOUNT_ID wired from the same variables — this is what unblocks L3.
2. Route pillar_agents._client through model_runtime.wrap, or fail fast on model_runtime.provider() with a distinct model_provider_unsupported marker.
3. Mark supporting_host_unknown in degraded (and use setdefault in _mark); keep failure accounting symmetric with _generate_evidence.
4. Add pillar_agents._clients / supporting._clients to the conftest autouse fixture; retry ReadTimeoutError in _structured; key presentation routing on f["id"].
5. Update report.py docstrings for the dual code path and document the three new env vars in the workers reference.

4. Verdict

Image manifest is empty (no staged HEAD images, no unavailable/omitted entries); both panel cells declared NOT_REQUIRED.

IMAGE_COVERAGE: NOT_REQUIRED

Blocking: the L3 unverified cross-batch IAM dependency (stays blocking by rule) plus the two verified L4 MAJORs.

Batch 5/7 — PASS

PR #245 — Batch 5/7 Chair Review (test_discovery.py, test_evidence.py)

1. Summary

This batch adds two offline test modules (scripts/v2/workers/diagnosis/test_discovery.py, test_evidence.py) for the new six-pillar diagnosis evidence loop; it contains no production code, IAM, Terraform, or docs. The modules under test (diagnosis/discovery.py, diagnosis/evidence.py, memory_metrics.py, workload_identities.py) are not in BASE but are all named in the other-batch path list, so their symbols are unverified cross-batch dependencies, not missing definitions (conftest.py already puts scripts/v2/workers on sys.path). Both panels found the same fixture defect; after verification against the diff and BASE, no finding in this batch's scope rises to CRITICAL/MAJOR.

2. Issues per lens

L2 — Code correctness

• MINOR (both models flagged; claude rated MAJOR, codex MINOR — chair verified in the diff and rules MINOR): In test_discovery.py's Database.run, the inventory_sync_runs branch counts from self.rows while the inventory_snapshots branch counts from the scope-filtered scoped. In test_member_reads_only_its_inventory_and_configuration_graph (1 host row + 1 member row, same kind) the ledger count is 2 while the snapshot count is 1, which — per this suite's own test_inventory_requires_same_attempt_and_reconciled_count contract — leaves member inventory accidentally unqualified. The test still passes because it never asserts freshness, so its scope-isolation assertions (only the member row, zero live calls, no finops/trace reads) remain valid, but the member-scope freshness contract is silently untested. Fix: count from scoped and add a freshness assertion. Test-only, no production behavior affected → not gating.
• MINOR (claude only, confirmed plausible): test_provider_requests_respect_sdk_resource_list_bound and test_inventory_cannot_starve_xray_service_nodes implicitly couple MAX_INVENTORY_RESOURCES ≤ MAX_RESOURCES − MAX_XRAY_RESOURCES without asserting the relation; a cap retune breaks one test obscurely.
• MINOR (claude only): max(db._usock.changes[:-1]) in the statement-timeout test raises ValueError on an empty sequence if the restore path regresses, producing an unreadable failure; the vacuous len(relationships) <= 400 assertion in the caps test (fixture produces zero relationships); positional evidence indexing (data["evidence"][2]…) in test_evidence.py that silently retargets on fixture insertion.

L3 — Security / AWS mutation safety

• No CRITICAL or MAJOR in this batch's scope. Both panels agree the batch strengthens the read-only boundary: the Provider fake exposes only read operations with no __getattr__ (mutations raise), _client is monkeypatched so no real boto3/credentials are reachable, the DIAGNOSIS_EVIDENCE_LOOP_ENABLED kill switch, budget-zero no-IO, host/member scope fail-closed, execute_sql rejection, and foreign-account/ARN non-admission are all positively pinned. Fixtures use documentation accounts and reserved IP ranges; no hardcoded secrets.
• Unverified cross-batch dependency (claude raised as MAJOR; chair excludes from this batch's gate): model_context() defaults to unredacted (private IPs/ARNs retained) with masking only under redacted=True, and nothing in this batch proves the Bedrock-bound callers use the redacted variant. BASE scripts/v2/workers/redact.py confirms redaction is mandatory before any Bedrock call. However, the callers (loop.py, pillar_agents.py, report.py) and the dedicated test_model_privacy.py are all listed in other batches, which plausibly provide exactly that caller-side proof; nothing in this diff violates the rule or weakens a control — this batch only adds redaction tests (including IPv6, hostnames, and mapping keys that BASE redact.py does not cover). The batch reviewing loop.py/pillar_agents.py/test_model_privacy.py must confirm the model boundary uses the redacted projection; if it does not, that batch blocks. Not a defect in this batch's diff.
• MINOR (claude only): two parallel redaction implementations now exist (diagnosis.evidence.redact_context vs workers/redact.py, whose BASE patterns lack IPv6/hostnames); the citation-id exemption from redaction has no pinned shape grammar, so a future id scheme could reintroduce scope text through the exempt channel.

L4 — Observability / data-integration correctness

• No gating finding. Codex found none; claude's three MAJORs are all missing-test-coverage observations about a test-only batch whose implementation lives in other batches — downgraded to MINOR suggestions after verification:
  • MINOR: no test bounds MetricDataQueries ≤ 500 per GetMetricData call (the CloudWatch API cap), while the analogous Compute Optimizer 100-ARN bound is tested.
  • MINOR (verified against BASE): scripts/v2/workers/finops/rules.py:352 treats both Overprovisioned and NotOptimized as EC2 savings opportunities, while line 399 restricts RDS to Overprovisioned only. The tests pin the RDS direction set and reject Underprovisioned/Optimized/None generically, but the EC2 NotOptimized direction is unasserted either way — add the case so the admission layer can't silently diverge from the rule engine the test comment cites.
  • MINOR: Compute Optimizer's regional scoping produces no asserted coverage disclosure for out-of-region resources, unlike every other omission in this suite.
• Positive parity verified by the claude panel against BASE (web/lib/graph-state.ts qualification matrix, inventory_read_mcp.py staleness fallbacks, the real EC2-vs-RDS Compute Optimizer field asymmetry, CloudWatch units/dimensions, the 200-pair LIMIT 201 sentinel) — spot-checked, accurate.
• MINOR (claude only): no happy-path assertion that degraded == [] / full native_sources shape; CloudWatch pagination (NextToken) untested; dual accepted dimension shapes (list vs mapping) widen the admission surface.

L5 — Docs / ADR consistency

• No gating finding (both panels agree). No docs change in this batch; the relevant docs (docs/reference/diagnosis-discovery.md, 06-workers.md, CHANGELOG.md, etc.) are listed in other batches — an unverified cross-batch documentation dependency, reviewed there.
• MINOR: the mutation-freeze contracts these tests encode (flag-off, deadline, execute_sql rejection) cite no ADR-005 reference anywhere in the new files; test_evidence.py lacks a module docstring; the literal "ORDER BY n LIMIT 201" SQL-substring assertion is formatting-brittle.

3. Suggestions

1. Fix the inventory_sync_runs fixture branch to count from scoped, and assert member-scope inventory freshness in the member test (the one defect both panels independently converged on).
2. Add the EC2 NotOptimized sizing-direction case and a len(MetricDataQueries) <= 500 chunking assertion on a many-resource fixture.
3. Assert the cap relation MAX_INVENTORY_RESOURCES <= MAX_RESOURCES - MAX_XRAY_RESOURCES explicitly, and replace positional evidence indexing in test_evidence.py with oid(...) lookups.
4. In the batch carrying loop.py/pillar_agents.py/test_model_privacy.py: require proof that every Bedrock-bound call uses model_context(redacted=True) (or make redacted the default), and reconcile redact_context with workers/redact.py so one pattern source covers IPv6/hostnames.

Review text truncated to fit the comment size limit; the complete batch reports are in the workflow log ("Batch review" groups of the chair step). The verdict above covers every batch.

---

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