[Atom-oh]

The combined dependency refresh updates Node to 26.13.0, Next.js to 16.4.5, React to 19.3.1, JavaScript AWS clients to 3.1161.0, Python workload runtimes to 3.14, Terraform to 1.17.1 and the documentation toolchain to Docusaurus 3.10.2. The author merged the former Python, Terraform and documentation branches into this PR; #224 is closed and this PR now contains their combined source.

Next.js uses proxy.ts with the existing body cap and a Webpack production build that retains strict application checks. ARM64 images, runtime HOSTNAME, authentication, ownership, default-off controls and release ordering are preserved. No manual deployment or Terraform apply is performed by this review loop.

The corrective pass incorporates current dev, preserving the reviewed MDX prerequisites and the follow-redirects fix, and removes an initiative plan from a namespace reserved for private-upstream documentation. Its exact bytes remain only in ignored local audit material. The unchanged native input filter admits the complete source at 127,817 bytes without source omission; review limits and workflows are unchanged by the fix.

Fresh host verification on this combined source: locked web and scripts installs passed under Node 26; production application TypeScript and the Webpack production build passed; documentation path validation passed. An in-memory error in actual web/lib/auth.ts produced TS2322 with source bytes unchanged, confirming strict application typing. Independent full inspection of both lockfiles covered 546 package entries and 1,343 dependency/peer edges without an actionable graph defect. These local checks do not replace fresh native review or current CI.

The earlier integrated snapshot was reported to pass 5,510 web tests (279 pre-existing optional skips), 333 Node CI tests and 73 disposable PostgreSQL tests. Those historical totals have not been rerun by this host on the corrective commit and are not represented as new receipts.

Remaining platform-reference corrections and bounded graph/type evidence are being prepared as a separately reviewed documentation prerequisite so the complete native input stays within its existing limit. Merge remains blocked until current CI, complete native dual-family review and independent publication-content clearance pass.

Split for review size (2026-11-05)

To keep the review input inside the 128 KiB bound, the documentation-site dependency refresh, its Node 26 guide workflow, README.md, docs/runbooks/dev-repo-setup.md, docs/architecture.md and docs/reference/* updates moved unchanged to #233, which merges after this PR. This PR keeps code, tests, CHANGELOG (refresh bullet under [Unreleased], released 0.10.1 entry restored), the runtime-dependency test prerequisites in CLAUDE.md/tests/CLAUDE.md, and the verification docs (docs/v2-merge-verification.md, runbook versions). Local check with Node 26.13.0 and Terraform 1.17.1: Merge Verify install set resolves, agent/Terraform/CI/web tests pass, next build succeeds, and tsc reports no errors beyond base dev's.

[github-actions]

🤖 AI Code Review (two independent reviewers)

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

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

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

Batch 1/8 — PASS

Both verification targets are resolved. The three-region assertion in the new tftest mirrors the existing line-322 pattern (mock aws_regions data), and runtime-read-scope.tf plus terraform/CLAUDE.md confirm "runtime regions" is the repo's established all-known-regions convention. The finops ECS/Lambda branches are untouched by this diff, and the comment-placement issue in workers.tf is confirmed but non-behavioral. I now have what I need to synthesize the chair review.

Chair Review — PR #227, Batch 1/8: six-pillar discovery diagnosis and trace instance observations

1. Summary

This batch adds two default-off, validation-chained Terraform gates (diagnosis_evidence_loop_enabled, diagnosis_network_evidence_enabled) granting only read/query-lifecycle IAM to the Fargate worker task role, hardens the CloudWatch/FinOps/flow-log MCP Lambdas with server-side input validation and evidence disclosure, extends prompt redaction to IPv6 and non-aws partitions, and fixes two real tool bugs (flowmonitor.py KeyError on gateway-wrapped arguments; ListMetrics rejecting MetricName=None). Both panels agree the ADR-005 read-only boundary holds with no CRITICAL findings; after chair verification against BASE, the one-model MAJOR claims do not survive as blocking (details below), leaving only MINOR issues.

2. Issues per lens

L2 — Code correctness

• MINOR (both models flagged) — scripts/v2/workers/handlers.py: summary["budget"] = budget.snapshot() is taken before the rpt.make_title_and_tags() fallback runs, so a fallback Bedrock call is excluded from the persisted budget ledger, contradicting the adjacent "Final ledger includes … local metadata selection" comment. Independent convergence by both panels; verified in the diff. Move the snapshot after meta resolution.
• MINOR (one model, verified) — handlers.py:69: function-local import time shadows the module-level import; any future time reference above it in _report becomes UnboundLocalError. Drop the local import.
• MINOR (one model, verified) — agent/lambda/network_mcp.py: minutes is unvalidated but now echoed as windowMinutes; a string raises TypeError (500), a negative value advertises a nonsensical window with empty events. Validate like _metric_request does.
• MINOR (one model, verified) — scripts/v2/workers/redact.py: the IPv6 pass over-redacts hex-colon strings that validly parse as addresses (de:ad:be:ef:00:01:02:03, ad::be, Python slice-like text). Verified this cannot cause a leak — _ipv6 returns the original on failure and the ARN rule cannot be consumed by the candidate regex — so the cost is corrupted prompt evidence text only.
• MINOR (one model) — web/app/api/dns-logs/analytics/route.ts, web/app/api/nfm/query/route.ts: let groups;/let status; rely on evolving-any inference through try/catch; an explicit type would be clearer. The 503-vs-404 semantic change itself is correct (a failed allowlist lookup is not proof of an unknown group/monitor).

Verified correct fixes worth recording: flowmonitor.py params['vpc_id'] → args['vpc_id'] fixes a KeyError on every AgentCore Gateway invocation (only the direct-invoke shape worked before), pinned by the new contract test; get_metric_metadata no longer builds MetricName=None (botocore-rejected); get_vpc_flow_logs truncation spread order is correct (explicit "truncated" key wins over **meta).

L3 — Security / AWS mutation safety

No CRITICAL or MAJOR. Both panels independently concluded the ADR-005 read-only boundary holds, and the chair's verification concurs: every added action is a read or an async query-lifecycle call; both new flags default false with validation chains (network ⇒ evidence_loop ⇒ workers); the member sts:AssumeRole is pinned to an explicit ≤32-entry AWSopsReadOnlyRole allowlist with a required ExternalId — strictly tighter than the existing task_assume_readonly wildcard pattern; logs:GetQueryResults/StopQuery on * matches the byte-for-byte precedent in workload.tf/workers.tf; no secrets enter env; frozen gates (remediation_enabled, integrations_write_enabled) are untouched and asserted off in the tftests.

• MINOR (downgraded from one model's MAJOR — chair verified against BASE) — variables.tf descriptions say grants are "limited to the runtime regions" while local.runtime_read_regions (runtime-read-scope.tf:74-85, all_regions = true) covers every known AWS region including future opt-ins, so the condition adds little geographic narrowing. Not blocking: "runtime regions" / runtime_read_condition is the repo's established term used by every existing read policy, terraform/CLAUDE.md explicitly documents "IAM includes all known regions (including future opt-ins)", and the new tftest's three-region assertion mirrors the pre-existing pattern at runtime_iam.tftest.hcl:322 (whose own error message says "including later opt-ins"). A precedent-consistent gated feature is not a new boundary violation. Suggested: clarify the two new variable descriptions ("all known regions, not a geographic restriction").
• MINOR (one model, verified) — worker_diagnosis_evidence/worker_diagnosis_network use for_each but hardcode role = aws_iam_role.worker_task[0].id, using each.key only in the policy name; a future "lambda" set member would silently double-attach to the task role.
• MINOR (one model) — the mandatory sts:ExternalId = "?*" condition fails closed for 1st-party member accounts whose registry external_id is NULL (allowed since the ADR-011 amendment migration). Fail-closed is the right direction; document the mismatch and make the runtime error distinguishable.
• MINOR (one model) — no validation ties diagnosis_member_role_arns/diagnosis_dns_log_group_arns to their enabling flags, so a populated allowlist with the loop off silently grants nothing.
• MINOR (one model, verified pre-existing for 21 of 24 keys) — tempo_mcp.py sanitizes sibling keys/AnyValue shapes only for the three instance-identity attributes; other allowlisted keys still pass raw entries through. Byte budget still holds; consider extending the projection.

L4 — Observability / data-integration correctness

Both panels confirm the core grounding changes are correct: unknown metric values stay None instead of round(0,4), flow-log configuration is now distinguished from traffic samples in tool descriptions and the agent prompt, window/truncation disclosure is added, and the EC2/RDS lookback/risk key asymmetry (lookBackPeriodInDays vs lookbackPeriodInDays, currentPerformanceRisk vs currentInstancePerformanceRisk) matches the API shapes.

• MINOR (downgraded from one model's MAJOR — chair verified) — aws_finops_mcp.py: _ranked_option/_recommendation_evidence apply only to EC2/RDS; the ECS (:100) and Lambda (:121) branches keep options[0] and carry no evidence keys. Not blocking: those branches are unchanged by this diff (pre-existing behavior), the catalog description accurately scopes evidence to "EC2/RDS include generation time, lookback, provider rank…", and the new agent.py cost rule explicitly instructs "absent risk, utilization or savings remains unknown" — directly countering the panel's postulated failure mode (model treating a missing performanceRisk key as absence of risk). Follow-up: extend _ranked_option to Lambda (its options do carry rank) and/or emit evidenceAvailable: false for ECS/Lambda.
• MINOR (one model, verified) — _utilization_metrics labels a provider response with no utilizationMetrics at all as utilizationTruncated: true; "absent" and "truncated" are different epistemic states. Also, a metric entry missing the optional statistic field is silently dropped and reported as a missing pillar, which feeds the downsize gate.
• MINOR (one model) — _metric_dimensions allows 30 dimension filters, but ListMetrics caps at 10; 11–30 pass local validation and fail server-side as a raw exception instead of the clean 400.
• MINOR (one model) — catalog.py: get_metric_metadata's description omits its 20-metric cap (the response does disclose returned/truncated); and dimensions is the first object-typed property without properties in any TARGETS inputSchema — unverified against a live AgentCore gateway; a rejection would fail make agentcore on the whole cloudwatch target. Worth one live smoke check before rollout.

L5 — Docs / ADR consistency

Both panels agree the bilingual CHANGELOG amendment is 1:1 and accurately describes the default-off, manual-proposal, member-never-substitutes-host posture; ADR-005's exception list (ADR-015, ADR-023) is untouched and nothing claims a new exception.

• MINOR (downgraded from one model's MAJOR — chair verified) — workers.tf: the new policies are inserted between the ADR-020 finops comment block (lines 886-893, "Add CE/COH/Budgets actions here ONLY when a rule that actually calls them ships") and the worker_finops_baseline resource it documents, so the governance comment now visually annotates the wrong policy — one that grants the same two Compute Optimizer actions under a different gate. No behavioral or policy change, so not gate-blocking, but fix the placement and give the new resources their own header comment.
• MINOR (one model) — agent.py renames analyze_reachability → check_reachability, leaving stale references in docs-site AI-assistant guides (3 locales) and the dark aws_core_mcp.py prompt helper; the new prompt-contract test guards agent.py only.
• MINOR (one model) — README's feature-flag table omits the four new variables; the table is a curated subset (not a convention violation), but these gates have non-obvious prerequisites (enabled registry row, non-null external_id, Resolver association, pre-existing NFM monitor) worth a row.
• MINOR (one model) — CHANGELOG does not record the query_flow_logs gateway-invocation fix, the get_metric_metadata namespace-only fix, or the redaction expansion (IPv6 + non-aws partitions); the last plausibly belongs as an amendment to the existing "Security scanning hardening" bullet.

Unverified cross-batch dependencies (non-blocking, named listed files plausibly provide them): handlers.py → rpt.evidence_loop_enabled(), diagnosis.budget.ReportBudget, the budget= kwarg on rpt.generate, summary["title_and_tags"]/summary["budget_incomplete"] (listed scripts/v2/workers/diagnosis/budget.py, report.py); benchmark-observation-cache.mjs → BoundedTtlCache (listed web/lib/bounded-cache.ts; note the script depends on Node type-stripping of a .ts import and hardcodes cache parameters that can drift); test_cloudwatch_metric_contract.py → jsonschema pin (listed scripts/v2/requirements-test.txt). Runtime enforcement of member-account registry/external-ID/assumed-identity checks depends on the listed diagnosis/scoped_clients.py (codex concurs).

3. Suggestions

1. Move budget.snapshot() after meta resolution in handlers.py and drop the shadowing local import time.
2. Relocate the new IAM resources below worker_finops_baseline (or above its comment) and add a dedicated header comment stating the diagnosis-evidence gate's own "exactly the actions the worker calls" rationale.
3. Clarify the two new variable descriptions: "all known regions including future opt-ins (runtime_read_condition), not a geographic restriction".
4. Validate minutes in network_mcp.py's get_vpc_flow_logs; cap ListMetrics dimension filters at 10; split "absent" from "truncated" in _utilization_metrics.
5. Add cross-flag validations (diagnosis_member_role_arns ⇒ evidence loop, diagnosis_dns_log_group_arns ⇒ network gate) and consider keying the new policies' role off each.key.
6. Smoke-test the object-typed dimensions schema property against a live gateway before rollout; update the docs-site analyze_reachability references and the README flag table; record the two tool fixes and the redaction expansion in CHANGELOG per the amend-in-place rule.

4. Verdict

No CRITICAL findings from either panel. The three single-model MAJOR claims were each verified by the chair against BASE and downgraded with cited evidence (precedent-consistent region convention per runtime-read-scope.tf:74-85 and runtime_iam.tftest.hcl:322; ECS/Lambda branches out of diff scope with accurate model-facing disclosure and an explicit absence-is-unknown prompt rule; comment placement is non-behavioral). All remaining agreed findings are MINOR. The manifest lists no images and no unavailable/omitted entries.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 2/8 — FAIL

The base file confirms the key disputed point: inventory_sync_runs is documented in scripts/v2/steampipe/sync_lambda.py:1349-1353 as a job-level ledger intentionally keyed under 'self' (one aggregator run covers all connected accounts), while per-account freshness lives on inventory_resources.captured_at and per-account inventory_snapshots. That resolves the panels' main disagreement — codex's member-ledger finding is a false positive. I verified the remaining MAJOR claims directly against the diff. Here is the synthesized chair review.

---

Chair Review — PR #227, Batch 2/8

Files in batch: scripts/v2/requirements-test.txt, scripts/v2/steampipe/sync_lambda.py (one projected column), scripts/v2/workers/diagnosis/budget.py (new), scripts/v2/workers/diagnosis/discovery.py (new, ~1200 lines).

1. Summary

This batch adds the diagnosis evidence-collection core: a thread-safe budget ledger and a bounded, code-owned discovery module reading Aurora inventory/graph/FinOps state plus read-only X-Ray/CloudWatch/Compute Optimizer APIs. The read-only (ADR-005) and cross-account scope postures are sound, but both panels independently found real correctness defects in coverage/freshness semantics, and several load-bearing guarantees (model-boundary redaction, the cited IAM Sid, the instance-observation producer) live in other batches and remain unverified cross-batch dependencies — the security/IAM ones stay blocking per the review rules.

2. Issues per lens

L2 — Code correctness

MAJOR — _metrics conflates "metric not published by this resource" with "window not covered" (claude flagged; verified in diff). ratios.append(... if good else 0) adds one entry per queried metric, and GetMetricData returns StatusCode: Complete with zero values for metrics the resource never emits (e.g. EBSReadBytes/EBSWriteBytes on pre-Nitro EC2, several AWS/RDS metrics on Aurora engines). ratio = min(ratios) then hits 0, so fresh=False and metrics_partial for the entire evidence row, including fully-covered CPU/memory series. missing_io/missing_memory already carry the not-applicable signal separately; non-applicable metrics should be excluded from the coverage ratio (engine/instance-family facts are available to select the metric set).

MAJOR — _finish cannot enforce MAX_BYTES when coverage itself is oversized (codex flagged; verified). The eviction loop pops only evidence → relationships → resources, then breaks. coverage.graph.*.sources projects up to 128 sources × 3 classes × ~11 fields of ≤128-char strings — this alone can exceed the 192 KB bound, leaving the bundle oversized despite the stated "JSON byte bound after all projections" contract and risking downstream prompt-allocation/export failures.

MAJOR — project_tasks failure inside _read_inventory's try misattributes and drops coverage (claude flagged; verified). from .workload_identities import project_tasks; project_tasks(bundle) runs after all inventory rows are projected but before coverage["inventory"] is assigned. Any failure there reports a successful inventory read as inventory_read_failed and skips the matched/returned/omitted disclosure and the inventory_cap mark — the one undisclosed-absence shape the module is designed to never produce.

MAJOR (unverified cross-batch) — _db_read closes the caller-owned connection on transport failure (claude flagged; verified in diff). Closing a desynced pg8000 stream is correct in itself, but the connection belongs to the report job; after the close, progress heartbeats and finish_report fail unless the caller reconnects. Whether scripts/v2/workers/diagnosis/loop.py/report.py (other batches) handle database_connection_broken cannot be verified here — confirm in those batches.

MINOR (claude): _finish's O(n²) full-bundle re-serialization per evicted row; write-only attachments_complete; a cancelled timeout-restore leaving a sub-second statement_timeout on the session (DatabaseError doesn't mark the connection broken); _xray labeling a non-integer ReferenceId as xray_node_cap; _arn hardcoding the aws partition and the rds fallback for all non-EC2 kinds; _provider_sizing silent on unanswered ARNs; follow_up/_metrics not re-checking DIAGNOSIS_EVIDENCE_LOOP_ENABLED; member_factory rebuilt per follow-up request (up to 4 AssumeRole calls).

Unverified cross-batch dependencies (non-blocking, plausibly provided by listed files): .workload_identities.project_tasks, .memory_metrics.collect, .scoped_clients.member_factory (matching listed diagnosis/*.py); jsonschema==4.26.0 justified by listed agent/lambda/test_cloudwatch_metric_contract.py; env wiring for DIAGNOSIS_EVIDENCE_LOOP_ENABLED/DIAGNOSIS_HOST_ACCOUNT_ID (absent from BASE — without it every self-scope collect() fail-closes as untrusted_host_scope) expected from listed terraform/foundation/workers.tf/variables.tf.

L3 — Security / AWS mutation safety

Both panels agree: no ADR-005 violation in this batch. All AWS calls are read-only (xray:GetServiceGraph, cloudwatch:GetMetricData, compute-optimizer:Get*Recommendations), all SQL is SELECT-only code constants with bound parameters, scope enforcement (12-digit validation, self/member XOR against DIAGNOSIS_HOST_ACCOUNT_ID, post-read storage != scope re-checks) is sound, no secrets.

MAJOR (blocking per cross-batch security rule) — unredacted account-scoped identifiers in model-bound facts; the scrub lives in other batches (claude flagged; verified). _facts intentionally keeps in-scope ARNs raw, _address keeps bare private IPv4s, and _read_graph writes raw db_host, instance_id, and source_id (the last not even through _text) into evidence facts. The asserted "specialist prompt boundary scrubs source strings before every model call" is enforced only by pillar_agents.py/presentation.py/redact.py in other batches — and redact.py itself changes in this PR. Stays blocking until confirmed there.

MAJOR (blocking per cross-batch IAM rule) — cited IAM Sid absent from BASE (claude flagged; verified: BASE workers.tf has no worker_diagnosis_evidence role or DiagnosisProviderSizingReadOnly Sid; the Compute Optimizer actions exist only on worker_finops_baseline without runtime_read_condition). The _xray/_metrics citations do reconcile with DiagnosisDataSourcesReadOnly. Confirm in batch 1 that the new grant is the two exact actions (not ai.tf's compute-optimizer:Get* wildcard) and that the Lambda compute path either gets the grant or is documented as degrading.

MINOR (claude): _text truncates before redacting, so a 12-digit account id split at the 240-char boundary escapes the redaction regex; trace sourceId strings are projected without the graph-state.ts vocabulary/enum validation onto a model-facing path.

L4 — Observability / data-integration correctness

MAJOR — _xray hardcodes fresh=True on truncated/failed service-graph reads (claude flagged; verified in diff). After xray_read_failed/xray_page_cap/xray_repeated_token/xray_node_cap set coverage["complete"]=False, every edge from the partial graph is still emitted with fresh=True — directly contradicting the module's own rule one function away ("Pages read is not data measured"). fresh is the gate every consumer uses.

MAJOR — Aurora readers are misclassified as standalone by the replica-source contract (codex flagged; verified against AWS semantics). aws_rds_db_instance.read_replica_source_db_instance_identifier is null for Aurora readers (cluster membership, not RDS read-replica linkage, determines their role), so _replica_source returns "none", no ReplicaLag spec is added (and the correct metric would be AuroraReplicaLag anyway), and no replica_source_unknown guard fires. Sizing evidence for Aurora readers omits replication health with no disclosure; the "synced null means standalone" comment is wrong for Aurora.

MAJOR — X-Ray identities structurally starved by the shared resource budget (claude flagged; verified structurally). With MAX_INVENTORY_RESOURCES=180 of MAX_RESOURCES=200 consumed before _xray runs, ~20 slots remain for service nodes; most edges then die on if not source_r or not target_r, while the paginator still spends deadline budget. Disclosed via resource_cap, but it functionally defeats the X-Ray source on any populated account.

MAJOR (unverified cross-batch, non-security — not independently blocking) — the entire instance-observation path depends on topology_edges.meta keys no BASE producer writes (claude flagged; verified: instance_observations*, instance_ref, database_ref, instanceObservationDrops appear nowhere in BASE). The plausible producer is the listed web/lib/trace-graph.ts; confirm in that batch, otherwise the PR's headline EC2→RDS observed_call capability is dead code.

Rejected — codex's inventory_sync_runs account_id='self' member-scope finding is a false positive. BASE sync_lambda.py:1349-1353 documents the ledger as intentionally job-level under 'self' (one aggregator run covers all connected accounts); per-account freshness is carried by inventory_resources.captured_at and the per-account inventory_snapshots, both of which discovery queries by :scope. The claude panel verified the same and defended it; the hardcoded 'self' matches the producer contract. (The aggregate row_count >= scoped_counts comparison is also direction-safe.)

MINOR: _xray slices Edges[:MAX_RELATIONSHIPS] per service with no truncation mark (codex rated MAJOR; downgraded — it needs >400 edges on one service in a 3-hour window, and the same path's fresh=True defect above is already blocking); metadataTruncated read from raw stored details cannot see the TS read-time derivation (claude); _provider_row hardcodes coverage.complete: True beside non-empty guard_hits (claude); _finops drops the upstream host-region-only coverage marker (claude); jsonb_agg order reliance on an ordered subquery (claude). The sync_lambda.py column addition itself is correct, and the null-preservation chain through jsonb_each → _facts → _replica_source holds for non-Aurora engines (claude verified; codex's Aurora caveat above is the exception).

L5 — Docs / ADR consistency

No ADR amendment implied; ADR-005 posture intact (both panels agree). MINOR (claude): docs/v2-merge-verification.md:197 and scripts/CLAUDE.md:222 now understate requirements-test.txt (neither file is touched in any batch); discovery.py's worker_diagnosis_evidence/DiagnosisProviderSizingReadOnly comment cites IaC absent from BASE (see L3); budget.py's "report.py imports these" docstring is present-tense for a state only the listed report.py batch can make true, and budget.MAX_OUTPUT_TOKENS=8192 vs report._MAX_OUTPUT_TOKENS deserves a disambiguating note. The Aurora "synced null means standalone" comment is inaccurate (see L4).

3. Suggestions

1. In _metrics, derive the per-resource metric set from engine/instance-family facts (or at minimum exclude zero-sample-but-Complete metrics from min(ratios)), keeping missing_io/missing_memory as the not-applicable channel.
2. In _xray, gate fresh on coverage["complete"] like every other collector, mark per-service edge truncation, and either reserve resource slots for service nodes or skip the X-Ray fetch when the budget is already exhausted.
3. Move project_tasks out of _read_inventory's try (own _mark reason), and have _finish trim or summarize oversized coverage.graph.*.sources under the byte cap.
4. Treat Aurora engines explicitly in _replica_source/_METRICS (detect engine prefix aurora, use AuroraReplicaLag, never report "none" from a null on Aurora readers).
5. In _db_read, surface database_connection_broken to the caller as a typed outcome rather than closing silently, or document the mandatory reconnect in loop.py/report.py.
6. For batch-1/other-batch verification: pin the Compute Optimizer grant to the two exact actions under runtime_read_condition, confirm the Lambda compute path, and confirm the prompt-boundary redaction tests (test_model_privacy.py) cover raw ARN/IP/db_host/source_id facts.
7. Redact before truncating in _text; update the two stale requirements-test.txt doc mentions.

4. Verdict

Verified blocking findings exist in L2 (3), L3 (2, cross-batch security/IAM — blocking by rule), and L4 (3). Image manifest lists no images and no unavailable/omitted entries.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 3/8 — FAIL

All verification done. BASE's shared redactor (scripts/v2/workers/redact.py:19) scrubs only IPv4; BASE finops/rules.py:352 confirms the EC2 NotOptimized parity the diff claims (with a botocore shape-contract test pinning that enum); BASE report.py redacts at every Bedrock call site (report.py:714), establishing the redact-at-LLM-boundary architecture. Here is the chair synthesis.

---

Chair Review — PR #227, batch 3/8 (diagnosis evidence/loop/memory-metrics/observations)

1. Summary

This batch adds the code-owned evidence-admission layer (evidence.py), the bounded two-pass analysis loop (loop.py), and two read-only collectors (memory_metrics.py, observations.py). The design is defensive and read-only throughout — strict ID/account/region admission, model numeric-assertion vetoes, default-off env gates, host-account-only scope, and query-lifecycle-only AWS verbs consistent with the ADR-019 read-pipeline precedent. The gate blocker is a redaction-contract gap: this batch newly admits IPv6 addresses into prompt-destined facts while the shared redactor at BASE scrubs only IPv4, and the fix (if any) lives in an out-of-batch file; per the cross-batch rule, security/redaction findings stay blocking until that batch proves otherwise.

2. Issues per lens

L2 — Code correctness

• MINOR (one model, claude — verified): observations._dns_binding filters local with r.get("facts", {}) but then indexes r["facts"] in the sources/targets comprehensions. Via the public project() entry with a DNS row lacking vpc_id (None == None matches a facts-less resource), this raises KeyError and aborts the stage instead of marking dns_binding_unresolved. The internal _dns path pre-filters, so impact is limited to the public entry point.
• MINOR (one model, codex — mechanism verified, trigger unverified): evidence._observation_supported confirms a single-resource observation from any cited complete metrics row that merely contains the resource in resource_ids; a multi-resource aggregate metrics row would qualify. No in-batch producer emits such rows (memory_metrics is per-resource) and the bundle is code-owned, so this is an unverified cross-batch dependency on scripts/v2/workers/diagnosis/discovery.py (listed) emitting only singleton metrics evidence. Note _sizing_supported is already strict (singleton inventory + exact dimension pinning) — the asymmetry supports hardening the observation path the same way. Downgraded from codex's MAJOR: no concrete failure trigger in the resulting code.
• MINOR (one model, codex — verified, downgraded): loop._fingerprint excludes bundle-level coverage/gaps/degraded/incomplete, so a follow-up that collects no new evidence but records an access failure is reported only as follow_up:no_progress, losing the collection failure reason. Fails safe (no findings change); diagnostics-only loss.
• MINOR (one model, claude — verified): after a follow-up pass, pillar_coverage for non-rerun pillars keeps the pass-1 findings count and completed status even when revalidation against next_index drops those findings. The coverage surface can overstate results.
• MINOR (one model, claude — verified): loop.run folds pass-1 index.downgraded into next_index.rejected (rejected_history), so a run with only downgrades still emits degraded: ["evidence:rejected"] — mislabeled telemetry.
• MINOR (one model, claude — verified): _admit_outputs's finding_limit branch appends only a degraded marker, unlike the invalid_output branch which also marks incomplete; a pillar zeroed by the findings cap is invisible in incomplete.
• MINOR (one model, claude — verified): loop._IDENTIFIER_TOKEN caps hex at 17 ([0-9a-f]{8,17}) so admitted summary-<24 hex> IDs are not stripped and their digits trip _NUMBER, rejecting valid review issues; the pattern also lacks re.IGNORECASE unlike _NUMBER. Relatedly, validate_finding applies _NUMBER to explanations raw (no identifier stripping), so ap-northeast-2/db.m5.large in an explanation is rejected as model_numeric_assertion while the review path deliberately tolerates the same tokens — fail-closed, but coverage-costing and undocumented.
• MINOR (one model, claude — verified): observations sets flow_matched = True before the unsupported-unit check, and nfm_rows_unmatched is marked even when every binding was skipped via the conflict continue (matching never attempted). Gap semantics are slightly wrong in both directions.
• MINOR (one model, claude — verified): memory_metrics.collect returns two shapes (metric key present only for selected resources) without documenting it; both _dns/_flows also burn a final reads.pause(attempt) on the last poll iteration, and the MAX_POLLS comment's "~7.75s" corresponds to seven pauses, not eight.
• NIT (claude): MAX_QUERIES = 6 is unreachable (2 monitors × 2 categories = 4); FACT_KEYS lists "instance_id" twice.

L3 — Security / AWS mutation safety

Both models agree no ADR-005 mutation violation exists in this batch — verified: every AWS verb is List*/Get*/Describe* or the start/poll/stop lifecycle of this worker's own Logs Insights / NFM queries (the same read-pipeline shape as the ADR-019 Athena entry, which CLAUDE.md classifies as ordinary GATED, not an owner-override). Collection is double-gated by DIAGNOSIS_EVIDENCE_LOOP_ENABLED/DIAGNOSIS_NETWORK_EVIDENCE_ENABLED, host-account-only (scope == "self" plus \d{12} fullmatch that correctly blocks the None == None case), the DNS log-group allowlist is account-pinned by strict ARN regex, Logs Insights query strings interpolate only _VPC_ID-validated IDs, ListMetrics sets IncludeLinkedAccounts: False with foreign-account vetoes, and no credentials or secrets appear.

• MAJOR — BLOCKING (one model, claude — gap verified at BASE; fix unverifiable in this batch): IPv6 bypasses the model-privacy redaction contract. observations._answers newly admits AAAA records (and IPv6 private_ip_address matching), landing IPv6 literals in facts["answer_ips"], which reach model prompts via redact_context → redact. BASE scripts/v2/workers/redact.py:19 scrubs only IPv4 ((?:\d{1,3}\.){3}\d{1,3}); the module header declares redaction before any Bedrock call "spec §9 mandatory". redact.py/test_redact.py are changed in another batch and may add an IPv6 pattern — unverified cross-batch dependency, and per the batch rule a security/redaction finding stays blocking until the batch containing redact.py confirms coverage.
• MAJOR — unverified cross-batch (one model, claude; codex found no L3 issue but did not trace this path): loop.run returns EvidenceIndex.admitted_bundle() — explicitly documented as unredacted, carrying full ARNs, private IPs, endpoints and DNS names — as "evidence_bundle" in the result. No model call exists in this batch, and BASE precedent (report.py:714 redacts ctx_json before _bedrock_render) plus the documented model_context(redacted=True) accessor indicate redaction belongs at the LLM boundary in pillar_agents.py/report.py (both listed in other batches). This is therefore a verification obligation, not a demonstrated in-batch defect — but because it is a redaction/security dependency, the cross-batch rule keeps it blocking until the batch holding pillar_agents.py/report.py/handlers.py shows every model prompt and persistence path consumes only redacted context.
• MINOR (one model, claude — verified): _project's secret-key denylist (secret|password|token|credential|raw|policy|user.?data) misses authorization, api_key, passwd, cookie, session, private_key, exactly where mapping=True admits arbitrary keys (coverage, nested metrics/dimensions/xray). Latent (producers are code-owned), cheap to widen.
• MINOR (one model, claude — verified): partition hardcoded as arn:aws: in _network_matches and _groups while the region regexes accept -gov-; GovCloud would fail closed (evidence loss, not a safety loss).
• Cross-batch IAM note (not gated here): the six IAM Sids cited in comments must land in terraform/foundation/workers.tf (listed in another batch); BASE's DiagnosisDataSourcesReadOnly has cloudwatch:GetMetricData but not ListMetrics. Batch 1 owns this verification.

L4 — Observability / data-integration correctness

• Rejected from gate (codex MAJOR — disproven against BASE): codex flagged EC2 NotOptimized as incorrectly treated as downsizing. BASE finops/rules.py:352 already filters EC2 on exactly ("Overprovisioned", "NotOptimized") ("Underprovisioned / Optimized are not cost-saving opportunities"), test_rules_shape_contract.py:26 pins the botocore EC2 Finding enum including NotOptimized, and RDS is Overprovisioned-only (rules.py:399) — exactly what _DOWNSIZE_FINDINGS mirrors. The diff's parity claim holds (claude independently verified the same lines); contesting the convention itself is out of this diff's scope. Retained as a suggestion only: _sizing_supported never compares current vs. recommended capacity, so documenting that sizing_candidate means "provider savings opportunity", not "verified downsize", would prevent misreading.
• Both models agree the DNS/NFM correlation layering is correct: observed_dns_resolution is excluded from _observation_supported's accepted kinds, carries binding: "dns_window_correlation" and coverage.complete: False — three independent vetoes keep window correlation at hypothesis level, matching the module docstring. _complete correctly separates freshness from completeness; _pressure_supported refuses declared missing_memory=False as a substitute for measured series. CWAgent mem_used_percent/Percent and the NFM topContributors field shapes check out (claude verified against the SDK shapes).
• MINOR (one model, claude — plausible): GetMetricData is called with MaxDatapoints exactly at the theoretical maximum (EXPECTED × len(queries)); a paginated response carries PartialData on non-final pages, and the all(status == "Complete") check would then discard a fully-recovered sample set. Fail-closed, but needlessly lossy.
• MINOR (one model, claude — verified): project()'s dns_rows[:MAX_ROWS * MAX_GROUPS] slice (400) is exactly tight against today's caps and is the one cap in the module not paired with an incomplete marker; raising MAX_GROUPS or queries-per-group would silently drop rows.
• MINOR: the post-follow-up pillar_coverage staleness (L2 above) is also an integrity-surface issue for report consumers.

L5 — Docs / ADR consistency

• No new mutation, autonomy, or owner-override is introduced, so the absence of a new ADR is correct (both models agree). Docstring-to-implementation consistency within the batch is good.
• MINOR (one model, claude — unverified cross-batch): the new default-off gates (DIAGNOSIS_EVIDENCE_LOOP_ENABLED, DIAGNOSIS_NETWORK_EVIDENCE_ENABLED, DIAGNOSIS_DNS_LOG_GROUP_ARNS, DIAGNOSIS_HOST_ACCOUNT_ID) appear in no BASE doc; the PR changes docs/reference/06-workers.md/diagnosis-discovery.md in other batches (plausibly covering the reference tier), but the top-level CLAUDE.md gated-surface inventory is not in any listed path. Cross-batch documentation coverage to confirm.
• MINOR (one model, claude): no runbook for the operator-supplied DIAGNOSIS_DNS_LOG_GROUP_ARNS allowlist, whose asymmetric semantics (wrong account invalidates the whole list; wrong region is skipped) match the kind of thing comparable gated features document under docs/runbooks/.

3. Suggestions

1. Confirm in the batch containing redact.py that an IPv6 pattern is added (and test_redact.py covers it); otherwise add one — this clears the blocking MAJOR.
2. In the batch containing pillar_agents.py/report.py/handlers.py, confirm every model prompt consumes model_context(redacted=True)/redact_context and that evidence_bundle is only rendered/persisted, never prompted.
3. Harden _observation_supported to require the metrics row's resource_ids == finding["resource_ids"] (singleton), mirroring _sizing_supported's inventory check.
4. Use r.get("facts", {}) in _dns_binding's sources/targets comprehensions.
5. Recompute (or mark stale) pillar_coverage for non-rerun pillars after follow-up revalidation; keep pass-1 downgrades out of rejected.
6. Extend _IDENTIFIER_TOKEN to the summary-<24 hex> form and add re.IGNORECASE; consider applying the same identifier stripping to finding explanations, or comment the intentional asymmetry.
7. Pair the dns_rows slice with an incomplete marker; move flow_matched = True after the unit check; drop the wasted final reads.pause; align MAX_QUERIES with the reachable maximum; dedupe FACT_KEYS.
8. Widen _project's sensitive-key denylist (api_key, authorization, cookie, session, private_key, passwd).
9. Derive the ARN partition from the region (or accept aws-us-gov) in _network_matches/_groups, or drop -gov from the region regexes for consistency.

4. Verdict

Two MAJOR items remain in the gate, both security/redaction-class cross-batch dependencies that the batch rules forbid downgrading on "another batch probably handles it" grounds: the IPv6 redaction gap (defect confirmed against BASE's redactor; fix unverifiable here) and the unredacted evidence_bundle export (verification obligation on the listed consumer files). All other panel MAJORs were downgraded or rejected after verification against BASE and the diff. Image manifest lists no images, no unavailable or omitted entries.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 4/8 — FAIL

I've verified the panels' load-bearing claims against the BASE tree and the diff. Synthesizing the chair review now.

---

Chair Review — PR #227, Batch 4/8 (six-pillar diagnosis: pillar agents, presentation, evidence-loop orchestration, member-scoped clients, supporting summaries, budget tests)

Summary

This batch adds the evidence-loop diagnosis path: six bounded Bedrock specialist calls plus a cross-pillar reviewer (pillar_agents.py), deterministic presentation (presentation.py), a gated _generate_evidence branch in report.py, member-credential isolation (scoped_clients.py), and host-account supporting summaries (supporting.py). The security posture is strong — read-only AWS calls, default-off env gate, model-output containment, prompt/data separation and redaction before send are all done carefully — but both panels independently confirmed one structural correctness defect (the required output schema is never communicated to the model), and several cross-batch IAM/env wirings this batch depends on cannot be verified here and remain blocking pending the batch-1 review.

L2 — Code correctness

MAJOR (both models flagged; chair-confirmed in the diff) — the JSON output contract is never sent to the model. pillar_agents._body(prompt, context, schema, capabilities) accepts schema and discards it: no tools, no output_config, no serialized schema in the system prompt. The prompts name only fragments of the contract (coverage_gap, the three capabilities, "at most eight findings"), yet _validate_shape enforces an exact key set (set(value) != set(schema["properties"]) → invalid_output) plus enum membership for pillar/severity/claim/verification, and invalid_output is not retried. The prompt string is assembled entirely inside this batch's analyze()/review(), so no cross-batch file (loop.py, evidence.py) can plausibly close this gap. Failure trigger: any specialist response with a reasonable but differently keyed shape fails validation on attempt 1; _generate_evidence then catches and ships analysis = {"findings": [], "degraded": ["analysis_failed"]} — an empty report. Independent convergence of both panels plus mechanical confirmation in the diff makes this the batch's anchor finding. Embed the schema in the prompt or use a structured-output/tool contract.

MAJOR (one model; chair-verified against BASE) — no progress heartbeat through the entire analysis phase. _generate_evidence emits once before loop.run(...) and only emits per-section after presentation.render has produced every body; the emit callback is not threaded into loop.run. BASE emitted per completed section precisely because reaper.py (verified: RUNNING_STALE_MIN default 60 min, keyed to update_progress advancing updated_at) fails rows with no heartbeat. At default budgets (~3300 s) the margin to reaping is ~5 minutes, erased by any raised DIAGNOSIS_TASK_BUDGET_S or lowered RUNNING_STALE_MIN; the UI also sits at 0/total for nearly the whole run. Thread the heartbeat into the loop (per pillar/pass).

MAJOR (one model; chair-verified in BASE, unverified cross-batch dependency on scripts/v2/workers/handlers.py) — database_connection_broken reaches a write on the broken connection. The new raise RuntimeError("database_connection_broken") propagates to BASE handlers.py:118–121, which calls ddb.finish_report(conn, report_id, status="failed", ...) on that same connection — the write the code comment says must never happen; the row then stays running until the reaper and the original error is masked. handlers.py is changed in another batch and may handle this; if it does not, this is a live defect.

MAJOR (one model; unverified cross-batch dependency on terraform/foundation/workers.tf / variables.tf) — DIAGNOSIS_HOST_ACCOUNT_ID silently disables all four supporting collectors. Verified: BASE workers.tf provides HOST_ACCOUNT_ID, and no DIAGNOSIS_HOST_ACCOUNT_ID exists anywhere in BASE. If batch 1 does not add it, supporting.collect's guard always fails and cost/posture/changes/commitment evidence is never collected, with no degraded/gaps marker distinguishing the skip from a provider failure. Independently of the wiring, the silent skip without a marker is a defect — add a supporting_host_unknown marker when the variable is absent.

MINOR (verified unless noted):

• pillar_agents._client keys its cache on an unquantized read_timeout that changes nearly every call; BASE deliberately quantizes (_CALL_TIMEOUT_STEP_S = 30) to keep the client cache small on an OOM-sensitive worker. Apply the same quantization.
• Throttling retry in _structured has no backoff (BASE _bedrock_render backs off), and the OutputTruncated retry re-submits the identical body with the same attempt_tokens, so deterministic truncation cannot recover.
• _serialize(..., allow_nan=False) runs in _bounded_body before the budget ledger engages; a non-finite float in discovery/observation facts raises a bare ValueError outside the ModelStageError/budget.record taxonomy.
• presentation._clip can reduce a single-long-token cell to just …[truncated], and _cell escapes after clipping, so a 700-char clip can expand several-fold.
• _facts/_evidence_invariant_sources use bare indexing (resource["kind"], edge["source"], etc.) outside any try; one malformed discovery row fails the whole report instead of degrading, unlike every other stage here.
• db.finish_report (verified BASE db.py:122) does json.dumps(summary) without default=str; the new summary embeds bundle["coverage"], model_stages and budget.snapshot() verbatim — one datetime/Decimal fails the terminal write. (budget.py is cross-batch; unverifiable here.)
• report.py now has module-level import copy while _render_intended_vs_actual (verified BASE line 365) binds a local copy = _INVARIANT_COPY... — harmless today, a latent shadowing trap.
• Progress labels ("Discovery", "Six pillars", "Assemble") ignore lang, diverging from the legacy path for the same report.
• test_ledger_reads_the_same_task_and_reserve_env_as_report runs subprocess.run(check=True) without capture_output, so failures surface with no stderr.

Cross-batch modules this batch imports that BASE lacks — diagnosis/budget.py, evidence.py, discovery.py, loop.py, observations.py — are all plausibly provided by paths listed for other batches; reported as unverified cross-batch dependencies, not missing symbols. Note report.py now imports .budget at module scope, so a broken budget.py breaks the legacy render path too; the DIAGNOSIS_EVIDENCE_LOOP_ENABLED gate does not isolate that risk.

L3 — Security / AWS mutation safety

The ADR-005 read-only guarantee holds in this batch (both models agree). Every new AWS call is a read (ce:GetCostAndUsage/GetReservationCoverage/GetSavingsPlansCoverage, securityhub:GetFindings, cloudtrail:LookupEvents, sts:AssumeRole/GetCallerIdentity, bedrock-runtime:InvokeModel); the whole loop is default-off behind DIAGNOSIS_EVIDENCE_LOOP_ENABLED. Model-output containment is sound: no tools, follow-ups constrained to three read capabilities over supplied resource IDs with no scope/query/action field, duplicate-key rejection, Markdown-hostile cell escaping that also prevents forging SIZE_MARK, mutating CLI in model prose withheld (not just marked) with the code-owned mark re-applied by _swept. The redacted serialization is the only one transmitted (the unredacted pass is a size pre-check only), and reviewer-projected findings are merged before redaction. STS credentials never reach logs; all error surfaces are fixed codes.

MAJOR (one model flagged; stays blocking per the cross-batch rule for IAM findings) — the sts:AssumeRole grant backing scoped_clients is new and unverifiable in this batch. Verified: DIAGNOSIS_MEMBER_ROLE_ARNS, diagnosis_member_role_arns, and Sid DiagnosisRegisteredMemberReadOnly appear nowhere in BASE workers.tf. The in-code allowlist, registry charset checks, post-assume principal check and GetCallerIdentity confirmation are all good, but they are only as tight as the IAM Resource list they mirror; if batch 1's workers.tf grants AssumeRole on a wildcard, the env allowlist is advisory. Batch 1 must show Resource pinned to exactly the configured member role ARNs, and an sts:ExternalId condition compatible with the legitimately-ExternalId-less first-party case (the migration-ULID citation for optional ExternalId is accurate — ADR-011 as amended). Codex found no security issue because the IAM lives in batch 1; this is a scoping disagreement, not a substantive one.

MINOR: the member session factory builds a client for any service/region (BASE precedent sg_rule_scan pins the service) — pin the services discovery needs and validate region against configured runtime regions; set_config('statement_timeout', ..., false) is session-scoped and a failed finally restore persists it on the shared connection (prefer transaction-local); the Config built at the top of member_factory is dead code.

The host-scope guard in supporting.py is correct (triple check: scope == "self", storage_scope == "self", account equals configured host), plus LINKED_ACCOUNT/AwsAccountId filters and per-row scope-mismatch rejection. No hardcoded secrets.

L4 — Observability / data-integration correctness

No Steampipe/CIS code is in this batch. The missing output-schema contract (L2) is also the dominant L4 defect — specialist/reviewer results cannot be reliably admitted (both models). Beyond that, both panels found the data integrations honest, and I concur: the Cost Explorer exclusive-End/first-of-month handling is correct, Estimated is read at the right level, mixed currencies gap rather than sum, pagination caps are disclosed (*_page_cap, repeated-token detection), Security Hub matches BASE parity (worst-first sort with unsorted fallback disclosed, InvalidAccessException = not-subscribed steady state), single-region reads never claim scope-wide coverage, and _evidence_invariant_sources deliberately keeps the legacy invariant evaluator's unresolved-target contract (partial: True, to_ref edges) so verdicts stay unknown instead of being falsely certified.

MINOR (one model each; verified in diff): prose withheld by _safe_explanation (numerics/URLs/fences/CLI words) is silently dropped with no marker or summary counter, so "no hypothesis offered" and "hypothesis suppressed" are indistinguishable; _safe_explanation's CLI vocabulary is narrower than _has_mutating_cli (misses eksctl, helm, psql, …) — share one list; the documented 4-prerequisite cap is enforced only on the reviewer projection, not in _validate_shape (64×2000 chars admitted and fully rendered); _commitment mixes row shapes and seeds more from an RI token whose pages are never fetched.

L5 — Docs / ADR consistency

Consistent with the cited register: ADR-005 (reads only, default-off gate, no model execution surface), ADR-011's amended optional-ExternalId rule (correctly cited by migration ULID), and the budget unification pinned by test_ledger_reads_the_same_task_and_reserve_env_as_report in a fresh interpreter — a good regression guard against the two clocks diverging.

MINOR: generate()'s docstring is stale — it describes only the legacy pipeline and omits the budget parameter and the evidence_loop_enabled() branch that bypasses everything it documents. MINOR (cross-batch): three new env variables (DIAGNOSIS_EVIDENCE_LOOP_ENABLED, DIAGNOSIS_MEMBER_ROLE_ARNS, DIAGNOSIS_HOST_ACCOUNT_ID) appear nowhere in BASE code/terraform/docs; batch 1 must declare them default-off and the member-onboarding runbook (docs/runbooks/onboard-target-account.md, untouched by any batch) does not mention the new allowlist step, so a registered member absent from the allowlist fails with member_role_not_allowed and no operator-facing explanation.

Suggestions

1. Transmit the output contract: serialize FINDINGS_SCHEMA/REVIEW_SCHEMA (or at least the key names and enum literals) into the system prompt, or adopt a tool/structured-output call; stop passing an unused schema to _body.
2. Thread the progress callback into loop.run (per pillar and per pass) so the reaper heartbeat and UI progress survive the analysis phase.
3. Reuse HOST_ACCOUNT_ID or mark the skip when DIAGNOSIS_HOST_ACCOUNT_ID is unset; add a withheld-prose counter to the summary.
4. In batch 1, confirm the DiagnosisRegisteredMemberReadOnly statement pins Resource to the exact member role ARNs and handles the no-ExternalId case; register the three new env flags default-off and document the allowlist step in the onboarding runbook.
5. Quantize the Bedrock client-cache timeout key and add backoff on throttling, matching BASE _bedrock_render.

Verdict

Confirmed MAJORs exist (schema contract never transmitted — both models, chair-verified; heartbeat gap; broken-connection terminal write and host-account env wiring pending cross-batch verification; IAM AssumeRole scoping unverifiable in this batch and blocking per the cross-batch security rule). The image manifest lists no images, no unavailable entries and no omissions.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 5/8 — PASS

All verification done. Codex's L4 MAJOR is a false positive (RDS Compute Optimizer genuinely uses instanceRecommendationOptions/dbInstanceClass/instanceFinding/lookbackPeriodInDays — distinct from EC2's recommendationOptions/instanceType), codex's L3 MAJOR is a test-fake fidelity concern that partially overstates the gap, and claude's two MAJORs are cross-batch dependencies on listed files (report.py/discovery.py, workers.tf), both fail-closed. Here is the synthesized review:

---

Summary

Batch 5 is test-only: four new offline pytest modules (test_discovery.py, test_evidence.py, test_evidence_report.py, test_inventory_identities.py) that pin the six-pillar discovery/evidence/report contracts — scope isolation, fail-closed host identity, read-only follow-up capabilities, redaction, sizing-evidence qualification and byte/pair caps. The contracts encoded are strongly aligned with ADR-005's read-only posture (mutating model output is rejected or withheld, the evidence loop is env-gated and payload-unflippable), and I verified in BASE that every diagnosis.* module these tests import is absent from BASE but listed as added in other batches of this PR, so no missing-definition finding stands. No CRITICAL/MAJOR survives within this batch's scope; the gate-relevant risks live in other batches' files and are flagged below as unverified cross-batch dependencies.

Issues per lens

L2 — Code correctness

• Unverified cross-batch dependency (gate-excluded here; must be checked in the batch owning scripts/v2/workers/diagnosis/report.py/loop.py) — one model (claude) flagged MAJOR; verified as real but out of this batch's diff. Production passes the payload account verbatim (handlers.py:69 → report.generate(conn, account, …) at handlers.py:94-96, and this batch's handler tests use {"account": "self"}), while the new contracts require an exact 12-digit account: test_arn_account_or_region_mismatch_is_not_admitted asserts EvidenceIndex(bundle(), "self") raises ValueError, and test_host_scope_requires_applied_exact_host_identity degrades any account not equal to DIAGNOSIS_HOST_ACCOUNT_ID. Every test here that drives report.generate(object(), "self") stubs out discovery.collect and loop.run, and both end-to-end tests use a 12-digit account, so "self"-resolution is never exercised through the real path. The resolution plausibly lives in report.py (listed in another batch); failure mode is fail-closed (degraded/failed report, not data leakage). The reviewer of that batch must confirm the resolution and ideally request an enabled-path test with account="self".
• MINOR (claude) — fake-DB inconsistency: Database.run's inventory_sync_runs branch counts self.rows while inventory_snapshots counts scoped, so the only mixed-account fixture can never reconcile counts; member-scope inventory freshness is effectively untested. Use scoped in both branches.
• MINOR (claude) — BrokenDatabase.run double-appends to self.queries (once directly, once via super().run); harmless today, fragile for future count assertions.
• MINOR (claude) — heavy cross-module imports between test files (test_evidence_report → test_presentation/test_discovery/test_evidence) make fixture helpers a de-facto API across batch boundaries; renames elsewhere break collection silently.
• MINOR (claude) — dead branch in Provider.get_paginator (Pager(self.metrics) unreachable); test_invalid_address_is_not_preserved_as_an_internal_identity accepts two contracts (None or "<ip>") without pinning one.

L3 — Security / AWS mutation safety

Both models agree there is no ADR-005 boundary break: the fake AWS provider exposes only read operations, model-proposed actions (terminate-instances verification, safe_downsize claim, delete-db-instance prerequisite) are rejected or withheld with _MUTATING_MARK, follow-up capabilities are allowlisted/single-pass/capped, and test_payload_cannot_enable_evidence_path proves the job payload cannot flip the gate. No secrets; all identifiers are placeholders.

• MINOR (downgraded from codex MAJOR) — the fake Database.run applies the account filter itself from the scope kwarg, so a real query that binds the scope parameter but omits the SQL predicate would still look scoped in these tests. Partially overstated by codex: a real query that fails to pass the scope kwarg at all would default to "self" and the member-isolation test would fail, so the parameter plumbing is covered — only predicate-in-SQL fidelity is not. The real SQL lives in discovery.py (another batch), where it must be checked directly; strengthening the fake to assert a scope predicate in the SQL text is a good follow-up.
• MINOR → unverified cross-batch dependency on terraform/foundation/workers.tf (batch 1) (claude flagged MAJOR; downgraded because the failure mode is fail-closed) — DIAGNOSIS_HOST_ACCOUNT_ID is a new, third host-identity env name; verified in BASE that workers standardize on AWS_ACCOUNT_ID/HOST_ACCOUNT_ID (sg_rule_scan.py:52, network_path.py:82, schedule_dispatcher.py:25, diagnosis/sources.py:539). The tests always set it explicitly, so a missing/misnamed Terraform env var yields zero resources with a green suite. Batch 1's reviewer must confirm the wiring; also no STS cross-check of the value, unlike agent/account_utils.py.
• MINOR (claude) — redaction assertions never check that a bare 12-digit account inside evidence facts (raw_account) is absent from the redacted model context (ARNs/IPs are covered).

L4 — Observability / data-integration correctness

• Dismissed (codex MAJOR — false positive) — codex claimed the RDS Compute Optimizer fixture invents instanceRecommendationOptions. The real GetRDSDatabaseRecommendations response does use instanceRecommendationOptions with dbInstanceClass, plus currentDBInstanceClass/instanceFinding/lookbackPeriodInDays — exactly what the fixture models; recommendationOptions/instanceType is the EC2 shape. The claude panel independently verified the same divergence as faithful. This is a case of agreement not existing: one model's contrary claim was checked against the API shape and rejected.
• MINOR (claude) — the ≤100-ARN request bound is asserted only for the EC2 Compute Optimizer path; the RDS fake ignores kwargs, so RDS chunking/parameter naming is unverified.

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 6cd8956b3cd381062f50b5c95bdf230db1399641 · workflow: .github/workflows/pr-review.yml

[Atom-oh]

The dependency/runtime refresh is split out to #240. After #240 merges, this PR will be rebased on dev so its diff is only the six-pillar discovery diagnosis and trace instance-observation work, and the review will run on that.

🤖 Generated with Claude Code

[Atom-oh]

Split into four smaller PRs so each review covers a coherent slice (the 8-batch review kept blocking on cross-batch dependencies): #242 (trace instance observations + agent tool contracts), then discovery/evidence core, DNS/NFM + supporting sources, and the pillar agents/loop/rendering wiring — opened in order as each predecessor merges. The combined result of the four branches is identical to this PR's head 6cd8956b. Closing this PR in favour of the split.

🤖 Generated with Claude Code