[Atom-oh]

Part B of the #227 split (A = #242, merged). Adds the default-off discovery and evidence core for the diagnosis worker. Nothing in the report path uses these modules yet; C and D wire them in.

Changes

• scripts/v2/workers/diagnosis/: bounded scoped discovery (discovery.py), evidence admission index and redaction-aware model context (evidence.py), report budget (budget.py), workload identities, measured EC2 memory metrics, and scoped member-account clients.
• scripts/v2/workers/redact.py: redaction updates used by the evidence context.
• scripts/v2/steampipe/sync_lambda.py: RDS read-replica source column in the inventory sync.
• terraform/foundation/{variables,workers}.tf: default-off worker flags diagnosis_evidence_loop_enabled and diagnosis_network_evidence_enabled, plus their read-only IAM/env wiring. runtime_iam.tftest.hcl covers both states.

Safety

• Both flags default to false, so plan shows no changes and costs $0. All new AWS calls are read-only (ADR-005 unchanged).
• No security group, VPC or public ingress changes.

Tests

• python3 -m pytest -q scripts/v2/workers/diagnosis scripts/v2/workers/test_redact.py: 967 passed, 11 skipped
• terraform fmt -check -recursive: clean

🤖 Generated with Claude Code

[github-actions]

🤖 AI Code Review (two independent reviewers)

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

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

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

Batch 1/5 — FAIL

Chair Synthesis — PR #246, Batch 1/5 (diagnosis discovery & evidence core: Terraform IAM gates, CI runtime-policy exemption, redaction, budget ledger)

1. Summary

This batch adds two default-off, read-only diagnosis IAM gates (diagnosis_evidence_loop_enabled, diagnosis_network_evidence_enabled) with allowlisted member sts:AssumeRole and DNS-log reads, a matching foreign-ARN exemption in ci_runtime_policy.py, IPv6/host redaction hardening, a new ReportBudget ledger, and one Steampipe column. The ADR-005 read-only boundary is preserved (both panels agree; chair verified no mutating action and all frozen flags untouched), but the new CI-gate exemption for cross-account AssumeRole is provably weaker than the sibling exemption it imitates, the new hostname redactor destroys regional AWS service endpoints in diagnosis prompts, and the feature ships with stale README/CHANGELOG and a false synchronization comment in budget code. I verified each adopted finding directly against the base tree and the resulting patched code.

2. Issues per lens

L3 — Security / AWS mutation safety

• MAJOR — The member-ARN exemption in ci_runtime_policy.py is weaker than the sibling target exemption and its "attached only to the worker role" proof is evadable. Both models independently flagged this surface from different angles; chair verified all three components against the base file:
  (a) No fence (claude): the pre-existing runtime_verification_targets exemption is gated at check_plan (base scripts/v2/ci_runtime_policy.py:337-340) on target == "dev", scope == "full", the verified runtime profile and host-only disabled. The new members exemption has no equivalent — it is honored for all four DEV_TARGETS (dev, atomoh, ssminji, whchoi) at any scope, and never checks that diagnosis_evidence_loop_enabled/workers_enabled are true in the plan.
  (b) Evadable sole-attachment proof (codex MAJOR, corroborated by a claude MINOR): _member_policy_attached_only_to_worker scans only configuration.root_module.resources for the literal string aws_iam_policy.worker_diagnosis_evidence. An attachment that consumes the policy via a hardcoded (predictable, host-account) policy ARN, a local, or a child-module resource is invisible to the scan, while the designated attachment keeps consumers equal to the expected set — so the cross-account sts:AssumeRole policy could additionally be attached to e.g. the web task role and still pass (_check_accounts sees only the host account in a hardcoded policy ARN). The module's existing recursive _resources() helper (base line 159, already used for configuration traversal at line 221) is not used here.
  (c) Ungated task-definition branch (claude): _check_member_positions requires attached only in the aws_iam_policy branch; the aws_ecs_task_definition branch exempts foreign ARNs in DIAGNOSIS_MEMBER_ROLE_ARNS on partial plans where the attachment proof was never evaluated.
  The shipped Terraform itself is correctly scoped; this is a weakening relative to the gate's own established standard for exempting foreign-account ARNs, on the highest-sensitivity exemption class (cross-account AssumeRole). Per the project rule to flag weakened controls, this blocks without a runtime trigger.
• MINOR — aws:RequestedRegion via local.runtime_read_condition enumerates every region (data.aws_regions with all_regions = true, base runtime-read-scope.tf:82-85), so the condition on sts:AssumeRole/Compute Optimizer/cloudwatch:ListMetrics is satisfied everywhere. Consistent with house pattern, so not a regression (claude only). See the L5 MINOR on the description wording.
• MINOR — sts:ExternalId StringLike "?*" requires only a non-empty external ID; the real binding is the member trust policy plus the runtime registry row. Worth a Sid comment so it isn't mistaken for value pinning (claude only).
• MINOR — diagnosis_dns_log_group_arns validation pins the account but not the region (logs:[a-z0-9-]+), and the all-regions condition won't constrain it (claude only).
• Agreed positives (both models; chair spot-checked): no mutating action anywhere; networkflowmonitor query lifecycle and logs:StartQuery-scoped + GetQueryResults/StopQuery-on-* follow existing in-repo read-only precedents; all four new variables default off; remediation_enabled/integrations_write_enabled untouched; member grant is strictly tighter than the existing arn:aws:iam::*:role/AWSopsReadOnlyRole wildcard precedent on the same role; no hardcoded secrets.

L2 — Code correctness

• MINOR — ReportBudget tier attempts ceilings are unreachable for full-size attempts (claude flagged MAJOR; codex found nothing; chair downgraded). Verified arithmetic: the per-attempt holdback of MAX_REVIEW_TOKENS = 16192 means light (100k tokens) admits at most 10 full Sonnet attempts of its declared 20, deep at most 11 Opus attempts of 32. Downgraded because the behavior is fail-closed and conservative, and small-token attempts can still reach the attempt ceiling — the limits interact rather than one being dead — but the declared tier attempt counts are misleading and the raised error will nearly always be "reserved output ceiling" instead of "attempt ceiling."
• MINOR — duplicated, partially divergent budget constants (claude flagged MAJOR; chair downgraded). Verified: base report.py:129-130,175,182 independently parses the same three env vars (identical defaults, so they cannot diverge at runtime in one process), but budget.MAX_OUTPUT_TOKENS is hardcoded 8192 while report._MAX_OUTPUT_TOKENS is env-configurable (default 32000). Today every legal section budget fits (min(8000+h, 32000) ≤ 8192+h holds for all headroom values — chair re-derived this, so claude's claimed env-only failure trigger does not actually exist), leaving a 192-token-margin maintenance hazard, not a current bug. Fix by importing from report.py. The false comment claiming the import exists is treated under L5.
• MINOR — no clamp on TASK_SECONDS/EXPORT_RESERVE_SECONDS: a reserve larger than the task budget (or a negative value) makes every reserve() fail as "analysis deadline exhausted," indistinguishable from real exhaustion; report.py clamps its equivalents (claude only).
• MINOR — member_roles() raises TypeError on unhashable list elements (set(value)), escaping the ValueError contract for library callers of check_plan (claude only).
• MINOR — a legitimate attachment replacement (["delete","create"]) makes attached = False, which then fails the policy check with the misleading "Invalid diagnosis member role position" (claude only).
• MINOR — IPV6_CANDIDATES is O(n²) on long hex/colon runs in attacker-influenced tag/log text inside the render deadline; cap candidate length or remove the body ambiguity (claude only).
• Chair-verified non-issues: the IPv6 pre-pass cannot corrupt ARNs (lookbehind + 2-colon-group minimum; the s3:::-at-token-boundary case is absorbed by the <arn> rule); no duplicate worker env keys (GRAPH_REBUILD_INTERVAL_MINS/INVENTORY_STALE_AFTER_MINUTES exist only in the web task def in base; the tftest would catch duplication); all tftest-referenced variables (incident_lifecycle_enabled, graph_rebuild_interval_mins, inventory_stale_after_minutes) and locals (runtime_read_condition, runtime_read_regions, acct) exist in base.

L4 — Observability / data-integration correctness

• MAJOR — the new <host> redactor over-redacts regional AWS service endpoints (claude flagged MAJOR; codex explicitly found no L4 issue — chair verified the mechanism and adopts it). The post-region group (?:[A-Za-z0-9-]+\.)* is zero-or-more, so sts.ap-northeast-2.amazonaws.com, sqs.ap-northeast-2.amazonaws.com, bedrock-runtime.… all become <host>: a botocore EndpointConnectionError reaches the diagnosis LLM as Could not connect to the endpoint URL: "<host>", erasing exactly the which-service signal a connectivity/VPC-endpoint diagnosis needs, across the most common hostname class in diagnosis evidence. The code's own rationale comment ("region-qualified names name account resources") is factually wrong for service endpoints. Changing * to + separates resource endpoints (always a service label after the region) from service endpoints, with a dedicated \.s3[.-] arm for S3 virtual-host names which would otherwise stop being redacted.
• MINOR — rule ordering: <host> runs after <acct>/<ip>, so a hostname containing a 12-digit run is left half-mangled (no leak — the identifier is already gone — but inconsistent output) (claude only).
• MINOR — ec2-54-1-2-3.compute-1.amazonaws.com and ip-10-0-1-5.….compute.internal escape both the host and IPv4 rules, carrying dashed-form addresses to the LLM (claude only).
• MINOR — read_replica_source_db_instance_identifier is three-shaped (bare id / cross-region ARN / NULL for both primaries and Aurora readers); the column addition is schema-safe (JSON data column, no test pins the column list), but a boolean "is replica" consumer would misclassify Aurora readers. Unverified cross-batch dependency on scripts/v2/workers/diagnosis/evidence.py / observations.py (claude only).
• MINOR — unverified cross-batch dependency: the network policy grants ListMonitors + the four *MonitorTopContributors actions but not ListScopes/GetMonitor (which the web role has); if scripts/v2/workers/diagnosis/observations.py (other batch) calls those, it takes a runtime AccessDenied (claude only).

L5 — Docs/ADR consistency

• MAJOR — README (EN + KR flag tables) and CHANGELOG are not updated, and neither file appears in this batch or the listed other-batch paths (claude flagged; codex found nothing — chair verified both files and the batch path list). The README flag table documents every comparable default-off gated feature (finops_baseline_enabled, sg_rule_activity_enabled, network_path_check_enabled) in both languages; this PR adds four operator-facing variables — one granting cross-account sts:AssumeRole — with no row. No existing [Unreleased] feature bullet covers the new evidence-loop/network-observation gates, so per the repo's own CHANGELOG convention this is a missing entry, not a covered amendment. Only docs/reference/06-workers.md is in another batch, so the docs are stale at merge.
• MAJOR — budget.py's comments assert a coupling that does not exist: "report.py imports these so the render deadline and this analysis ledger cannot desynchronize" and "report.py shares this parse." Chair verified report.py is unchanged by this PR (absent from this batch and the other-batch list) and owns independent parses (report.py:129-130,175,182-187); budget.py is a new file no shipped module imports. The comment is the module's stated safety contract and it is false — the two ceilings are already divergent (8192 hardcoded vs env-configurable 32000). Either actually import from report.py (the pattern propose.py uses) or correct the comments to state the values are duplicated and hand-synced. (claude only; chair-verified.)
• MINOR — "limited to the runtime regions" in both new variable descriptions oversells the constraint (the condition enumerates all regions); the README's existing "known regions" phrasing is accurate (claude only).
• MINOR — ADR-005/007/etc. bodies are not in this tree (only docs/decisions/023-… exists), so ADR-vs-implementation diffing is only possible against CLAUDE.md/terraform/CLAUDE.md statements. Pre-existing gap, not introduced here (claude only).
• Agreed positive (both models): the four variable descriptions otherwise match the generated policies statement-for-statement, the managed-vs-inline quota rationale is sound, and the descriptions correctly frame the feature as bounded, read-only, and default-off.

3. Suggestions

1. ci_runtime_policy.py: fence the member exemption like its sibling (require full dev scope and diagnosis_evidence_loop_enabled is True); rewrite _member_policy_attached_only_to_worker to use the recursive _resources() helper and to also reject any plan/configuration attachment whose policy_arn string could resolve to the exempt policy (name match), and require attached in the task-definition branch too; accept ["delete","create"]/["create","delete"] attachment replacement.
2. redact.py: change the post-region group to +, add an S3 virtual-host arm, move <host> ahead of <acct>/<ip>, and consider compute(-\d)?\.(internal|amazonaws\.com) coverage; bound the IPv6 candidate length.
3. budget.py: import the env-derived constants (and the output ceiling) from report.py, or fix the comments; clamp TASK_SECONDS/EXPORT_RESERVE_SECONDS; reconcile tier attempts with the holdback math.
4. Add README flag-table rows (EN+KR) and a CHANGELOG [Unreleased] bullet for the new gates; mirror the member-ARN "requires the loop" validation onto diagnosis_dns_log_group_arns, and pin its region to var.region.

4. Verdict

Image evidence: the manifest is status: complete with zero images, zero unavailable entries and zero omissions, and this batch touches no image file; both panel cells correctly declared NOT_REQUIRED.

IMAGE_COVERAGE: NOT_REQUIRED

Blocking: one MAJOR in L3 (weakened CI gate around the cross-account AssumeRole exemption — both models converged from independent angles, chair-verified), one MAJOR in L4 (service-endpoint over-redaction, chair-verified mechanism), and two MAJORs in L5 (stale README/CHANGELOG not fixed by any batch; false synchronization contract in budget comments).

Batch 2/5 — FAIL

All disputed claims are now verified against BASE. I have everything needed to synthesize the chair review.

Chair Review — PR #246, Batch 2/5 (scripts/v2/workers/diagnosis/discovery.py, new, 1321 lines)

1. Summary

This batch adds the default-off, code-owned evidence-collection core for the diagnosis worker: bounded reads of Aurora inventory/topology/FinOps tables plus read-only X-Ray, CloudWatch, and Compute Optimizer calls, with explicit coverage/freshness disclosure. The security posture is strong — every outward call is read-only, scope validation fails closed, and the ADR-005 frozen-mutation boundary is upheld — but three verified MAJOR correctness defects remain, all of the same class: silent data loss or a latent crash that contradicts the module's own "disclosed, never dropped silently / never fails collect()" contract. Codex's headline L2 finding (member-scope inventory_sync_runs) is a confirmed false positive refuted by BASE.

IMAGE_COVERAGE: NOT_REQUIRED

(The manifest lists zero images, zero deletions, zero unavailable/omitted entries, status complete; both panel cells declared NOT_REQUIRED.)

2. Issues per lens

L2 — Code correctness

MAJOR — Unguarded consumption of the memory-metrics result can abort all of collect() (claude flagged MAJOR; codex did not flag; chair verified in diff). In _metrics, the try/except wraps only the collect_memory(...) call; consumption happens later in the per-resource loop: ratios.append(observed["sample_ratio"]) and statuses.append(observed["metric"]["status"]) index the returned dict unguarded. A memory_metrics entry containing metric but lacking sample_ratio (or metric["status"]) raises KeyError out of _metrics → out of collect(), killing the entire diagnosis. This directly contradicts the inline contract two lines above ("its failure leaves EC2 memory unknown, never fails collect()") and the file-wide fail-soft pattern. memory_metrics.py is new in another batch, so the trigger depends on that module's shape contract — but the defensive gap and the broken stated invariant are in this batch's code, and the fix (.get() + the existing memory_metrics_read_failed mark) is trivial.

Unverified cross-batch dependency (potential MAJOR, excluded from this batch's gate) — (claude flagged). The deadline pre-gate in _pages and _provider_sizing compares the remaining budget against connect_timeout + read_timeout from the client's own config. The host-path _client() pins these to ~1+10s, but the member path uses clients from scoped_clients.member_factory (new file, listed in another batch). If that factory does not pin timeouts, botocore defaults (60+60s) make the gate remaining <= 120, which would skip every member-account CloudWatch/Compute Optimizer page as a spurious discovery_deadline. scripts/v2/workers/diagnosis/scoped_clients.py is listed and plausibly provides pinned configs; the batch reviewing it must confirm.

REFUTED — codex's MAJOR "member inventory uses host-account run provenance" is a false positive. Codex claimed inventory_sync_runs WHERE account_id='self' should be :scope; claude explicitly disputed this with base evidence, and the chair confirmed: BASE scripts/v2/steampipe/sync_lambda.py:1349-1353 documents inventory_sync_runs as a job-level ledger, one row per resource_type keyed under the 'self' sentinel, "intentionally NOT per-account," with per-account freshness carried by inventory_resources.captured_at — which this file checks separately (_fresh(captured, stale)), and per-scope counts correctly use :scope (inventory_snapshots, live count(*)). Codex's proposed fix would find zero ledger rows for members and permanently disqualify valid member evidence. Excluded from the gate. This is a textbook case of two models diverging where only one reconciled against the producer.

MINOR (claude; chair spot-checked, all plausible and non-blocking): recursive-CTE fan-out in the nodes query can approach the sub-10s statement_timeout on large graphs (fails closed to graph_read_failed); _finish re-serializes the ~192 KB bundle once per evicted row; _graph_max_age docstring overstates parity with web/lib/graph-state.ts:130-131 (chair confirmed TS has no 1440 clamp — Python is stricter/fails closed); multi-page GetMetricData status aggregation (all(s == "Complete") across pages) would zero sample ratios on a paginated response (unreachable at today's ≤32,256 datapoints vs MaxDatapoints=100800, but fragile); _db_read's finally issues a restoring set_config even when no new value was applied; _xray mislabels a non-integer ReferenceId as xray_node_cap instead of an unresolved-shape reason.

L3 — Security / AWS mutation safety

No CRITICAL or MAJOR. Both models agree the ADR-005 read-only boundary is upheld, and claude's exhaustive call inventory is confirmed: only xray:GetServiceGraph, cloudwatch:GetMetricData, compute-optimizer:Get{EC2Instance,RDSDatabase}Recommendations, and parameterized code-owned SELECTs (plus session-local statement_timeout set/restore). No mutation, no assume_role in this file, no DML/DDL, no hardcoded secrets. Scope isolation fails closed: 12-digit account validation, the (scope == "self") == (host == account) biconditional, _node_inventory account allow-listing, the unique-endpoint (sum(...) == 1) check on instance observations, and follow_up's single-pass/4-request/exact-key revalidation.

MINOR (claude; chair verified the BASE side):

• The DiagnosisDataSourcesReadOnly Sid cited for X-Ray/CloudWatch exists in BASE (terraform/foundation/workers.tf:861, :931). The cited worker_diagnosis_evidence Sid DiagnosisProviderSizingReadOnly does not exist in BASE — unverified cross-batch dependency on batch 1's workers.tf; BASE's own comment at workers.tf:886-892 sets the "exactly the actions the worker calls" precedent that batch must honor. If absent, this file degrades safely (provider_sizing_read_failed), assuming no privilege.
• DIAGNOSIS_EVIDENCE_LOOP_ENABLED / DIAGNOSIS_HOST_ACCOUNT_ID are absent from BASE Terraform — cross-batch; batch 1 must source the host id from a trusted Terraform value, not a request field. Default-off posture here is correct.
• Chair confirmed BASE redact.py:14 matches only arn:aws: — a gov/China-partition ARN falling through _facts' scope check to _text() keeps service/region/path (account still masked). Pre-existing gap; redact.py is changed in another batch and should be verified there.
• The raw-identity design (ARNs/IPs/db_host kept raw in the bundle) is acceptable only if evidence.py's model boundary (other batch) redacts every non-citation string, including facts["dimensions"], db_host, endpoint_address, instance_id, correlation_id.

L4 — Observability / data-integration correctness

MAJOR — X-Ray truncation can silently drop nodes/edges while coverage claims complete (codex flagged MAJOR; claude missed it; chair verified in diff). In _xray, (page.get("Services") or [])[:MAX_RESOURCES] slices before the cap check — a single page with >200 services drops the excess without ever entering the loop's else branch, so coverage["complete"] stays True and xray_node_cap is never marked (the mark fires only when overflow spans pages). Likewise (service.get("Edges") or [])[:MAX_RELATIONSHIPS] silently discards edges beyond 400 on one service node with no mark at all, and every evidence row from that page is emitted with fresh=coverage["complete"]=True. This violates the module's central contract — truncated evidence presented as complete coverage to the diagnosis model. Check lengths before slicing and mark the cap.

MAJOR — _provider_sizing silently omits resources whose ARN cannot be reconstructed (claude flagged MAJOR; codex did not; chair verified in diff). selected = [r for r in resources if ... and _arn(r)] drops any resource whose stored ARN failed _facts' scope check, disagrees with resource_id, or sits in an aws-cn/aws-us-gov partition (the synthesized fallback hardcodes arn:aws:) — with no gaps/incomplete marker, making "not queried" indistinguishable from "Compute Optimizer has no recommendation." Every sibling omission path discloses a reason (metric_identity_unresolved, provider_scope_mismatch, finops_scope_unresolved, …). Add a provider_identity_unresolved mark mirroring _metrics.

Chair also confirmed claude's positive verification of the inventory contract: resource-id conventions match the producer (sync_lambda.py ec2/rds id shapes), the row_count >= scoped_counts / point[1] == scoped_counts asymmetry is the correct global-ledger-vs-scope comparison, and the trace-loss vocabulary matches docs/api-reference.md.

MINOR (claude): coverage["resources"]/["evidence"] always report omitted: 0 because the cap branches in _resource/_evidence never increment a matched counter (unlike _relationship) — the numeric coverage contract is wrong even though incomplete discloses the cap; _inventory_source_fresh's docstring overclaims a full mirror of inventorySourcesStale (status/reasons checks live in the caller — composite result is equivalent, chair spot-checked graph-state.ts:141-147); _client's deadline TimeoutError surfaces as *_read_failed in degraded instead of discovery_deadline; _bound_coverage can exit above its byte share once all source lists are emptied; malformed scalar types in a trace sources row are normalized away without failing freshness; follow_up + graph_refresh can leave a stale and a fresh relationship per logical edge (document the resolution rule).

L5 — Docs / ADR consistency

No MAJOR. The batch adds no docs; both models agree the implementation is consistent with the frozen-mutation posture.

MINOR (claude): the redaction boundary is named three inconsistent ways in this one file (evidence.EvidenceIndex.model_context(redacted=True) vs evidence.redact_context vs "the specialist prompt boundary") — since this sentence is the justification for storing raw identities, reconcile to the real API name against evidence.py (other batch); DIAGNOSIS_EVIDENCE_LOOP_ENABLED/DIAGNOSIS_HOST_ACCOUNT_ID, the bundle shape, the _CAPABILITIES vocabulary, and the follow-up single-pass contract are undocumented in BASE — docs/reference/06-workers.md is listed in another batch and must cover them; the ~30 free-form gaps/degraded/incomplete reason strings have no module-level enumeration, unlike _REASONS; _replica_source's comment presumes a producer column (read_replica_source_db_instance_identifier) that BASE sync_lambda.py does not yet select — plausibly added by the listed sync_lambda.py change, degrades correctly either way. (ADR-005 having no committed body in this public tree is a known repo-wide property per CLAUDE.md, not a finding against this PR.)

3. Suggestions

1. Move the three memory-consumption lines inside the guarded block, or use .get() with the existing memory_metrics_read_failed mark, so the stated "never fails collect()" invariant actually holds.
2. In _xray, capture len(page.get("Services") or []) and per-service len(Edges) before slicing; mark xray_node_cap/a new xray_edge_cap and set coverage["complete"] = False on overflow.
3. In _provider_sizing, mark provider_identity_unresolved for resources dropped by _arn(r), mirroring metric_identity_unresolved.
4. Increment a matched counter in _resource/_evidence cap branches so coverage.*.omitted is truthful.
5. For the batches that own them: confirm scoped_clients.member_factory pins client timeouts compatible with the connect_timeout + read_timeout deadline gate; confirm batch 1's workers.tf adds DiagnosisProviderSizingReadOnly as exactly the two Compute Optimizer read actions under runtime_read_condition; confirm evidence.py redacts every raw identity field this batch stores; confirm redact.py gains aws-cn/aws-us-gov ARN coverage.
6. Unify the redaction-boundary naming in the module docstring and inline comments to the single real accessor.

4. Verdict

Three verified MAJOR findings stand in this batch (unguarded memory-result consumption that can abort collect(); silent X-Ray node/edge truncation reported as complete; undisclosed _provider_sizing resource omission). L3 is clean — the ADR-005 read-only boundary is intact — and codex's member-scope ledger MAJOR was refuted against BASE and excluded.

Batch 3/5 — FAIL

Verification complete. BASE scripts/v2/workers/redact.py confirms the REDACTORS list has an IPv4-only rule (no IPv6), and BASE finops/rules.py:352,399 confirms the _DOWNSIZE_FINDINGS parity claim (Overprovisioned|NotOptimized for EC2, Overprovisioned for RDS). I also re-checked the diff directly for the _TYPE_KEYS and _scrub_text claims. Synthesizing the chair report now.

Chair Review — PR #246, Batch 3/5 (diagnosis evidence core: evidence.py, memory_metrics.py, observations.py, scoped_clients.py, supporting.py, test_budget.py)

1. Summary

This batch adds the code-owned evidence admission/validation layer, scoped member credentials, bounded network-observation reads and account-summary collection for the default-off diagnosis feature. The read-only ADR-005 boundary holds — every AWS call enumerated across all five files is a read/list/query operation, the cross-account path is tightly fenced (registry + ExternalId + exact-ARN allowlist + principal and caller-identity verification), and error paths leak no provider data. However, the batch carries one verified redaction defect (IPv6 answer IPs bypass every scrub layer this batch owns, with the fix at best in an unseen batch), one verified model-context corruption bug (db_instance_class masked as <host>), and unresolved IAM-scoping dependencies on batch 1 for the new Start/Stop query verbs.

2. Issues per lens

L2 — Code correctness

MAJOR (chair-verified; claude flagged, codex did not): db_instance_class missing from _TYPE_KEYS destroys RDS class values in the model context — evidence.py. _HOSTNAME.fullmatch("db.r6g.large") matches (two dotted labels + alpha-initial TLD-like tail), db_instance_class is in FACT_KEYS and read by _sizing_supported, but is absent from the _TYPE_KEYS exemption, so redact_context rewrites every RDS class to <host> in model_context(redacted=True). Code-side sizing validation is unaffected (it reads unredacted facts), so this is corrupted model input rather than a safety break — but it is a verified logic bug in the new documented projection. Add db_instance_class (and audit graph_class) to _TYPE_KEYS.

MINOR (downgraded from claude's MAJOR; unverified external behavior): Unit: "Percent" on the CWAgent mem_used_percent GetMetricData query — memory_metrics.py. The panel claims the agent publishes this metric unitless by default, which would silently zero out all EC2 memory evidence and permanently downgrade every EC2 sizing_candidate. AWS's agent-metrics documentation lists mem_used_percent with unit Percent, contradicting the panel's premise, and this cannot be verified from the diff. The Unit filter is redundant anyway (the series is already pinned by namespace + name + exact dimension set via _metric): drop it or verify against a live account. If the panel is right, the failure reason memory_metric_samples_incomplete also misattributes the cause.

MINOR (claude):

• validate_request rejects rds_instance while _native_identity/_sizing_supported accept it, so a legitimate follow-up burns the single pass with unsupported_resource_capability — the exact outcome the inline comment says it prevents. Reconcile with what discovery.py (batch not seen here) actually emits.
• _admit_relationships validates kind against KINDS rather than _RELATIONSHIP_KINDS, admitting e.g. a metrics-kind relationship. Harmless today, but a semantic hole in a code-owned gate.
• supporting._posture's window = {"filter": "ACTIVE_NEW"} is dropped by _project (filter ∉ FACT_KEYS), so the model never learns the posture rollup is ACTIVE/NEW-scoped.
• Dead unused config local in scoped_clients.member_factory; observations.MAX_POLLS comment understates worst-case sleep (8 pauses ≈ 9.25 s, final pause always wasted); whole-batch failure attribution in memory_metrics (one CollectionGap mislabels already-complete resources — fail-closed, but misleading reasons); observations._qualified cross-check is vacuous when a resource carries none of the four identity facts; ValueError in _SOURCE_ERRORS misroutes dns_query_vpc_invalid (a code defect) into gaps instead of degraded; _commitment seeds more from the RI NextPageToken while only reading the complete Total aggregate, and omits Granularity on the RI call.

Codex reported no L2 findings; the panel disagreement on this lens is resolved in claude's favor by direct diff verification of the _TYPE_KEYS bug.

L3 — Security / AWS mutation safety

ADR-005 freeze: holds. Both models independently confirmed no mutating API, no remediation path, no frozen-flag toggle, no persisted credentials, no hardcoded secrets. The member-credential path (scoped_clients.py) is strictly tighter than the sg_rule_scan precedent it cites.

MAJOR (chair-verified premise; claude flagged — blocking per the cross-batch security rule): IPv6 answer_ips reach the model unredacted. observations._answers deliberately admits AAAA records into facts["answer_ips"]; in evidence.redact_context, answer_ips is not in _HOST_KEYS, _HOSTNAME cannot match a colon-bearing literal, _AWS_HOST does not apply, and I verified BASE redact.py REDACTORS carries only an IPv4 pattern. An IPv6 resolver answer therefore goes to Bedrock verbatim while its IPv4 twin becomes <ip>, contradicting both evidence.py's own docstring and redact.py's "scrub IPs before ANY Bedrock call" contract. scripts/v2/workers/redact.py is changed in another batch and may add IPv6 — but the cross-batch rule forbids lowering a security/redaction finding on that basis. Fix in this batch's own code (add answer_ips handling or an IPv6 rule) or confirm the redact.py batch closes it.

MAJOR (claude; unverifiable here, batch-1 reconciliation required): new Start/Stop query verbs need confirmed IAM scoping. logs:StartQuery/StopQuery and four networkflowmonitor:*Query* actions are the diagnosis tier's first non-Get/List/Describe verbs. The code-side allowlist (_groups: ≤8 ARNs, account-pinned regex) constrains only what the code chooses — it is not a substitute for Resource-scoping logs:StartQuery to the allowlisted groups in workers.tf, and the post-deadline _Reads.stop cleanup path must not be broken by a runtime_read_condition. The policy also needs consistent attachment to both the task and Lambda roles per the BASE dual-attachment precedent. All of this lives in batch 1 (workers.tf, runtime_iam.tftest.hcl, ci_runtime_policy.py); per the cross-batch rule an IAM finding stays blocking until that batch confirms it.

MINOR (claude): DIAGNOSIS_HOST_ACCOUNT_ID diverges from the existing AWS_ACCOUNT_ID convention and, if unwired in batch 1, silently inerts the whole feature (fail-closed, so not blocking); sts clients in scoped_clients.py are not region-pinned (global-endpoint + 1 s connect timeout + no retry = hard failure behind a VPC endpoint); statement_timeout restore in the finally can mask the original ScopeError; tag values (Owner, Environment, tags) reach the prompt — well-mitigated (1024-byte cap, mapping=False, numeric-assertion ban in validate_finding) and best recorded as accepted residual risk.

Codex found no L3 issues; it did not examine the redaction layering, so the disagreement reflects coverage depth, not a contested claim.

L4 — Observability / data-integration correctness

MAJOR (claude only; design-level, resting on documented ingestion-lag behavior): the 5-minute window ends at now. observations.collect queries [now-5min, now] with no publication-lag offset, while NFM top contributors and Resolver query-log delivery lag real time by minutes. The predictable result is empty reads misreported as source gaps (nfm_unavailable/nfm_rows_unmatched) rather than evidence. Fail-closed and honest, but likely to make the feature inert in practice; shift the window back by a lag allowance and widen the inventory-freshness coupling in step. Should be empirically confirmed before relying on it.

MINOR (codex MAJOR, chair-downgraded): _observation_supported accepts absent binding attestations. Codex is factually right that empty facts pass both negative checks (binding != "dns_window_correlation", resource_binding != "unresolved"). But the evidence bundle is produced by trusted discovery code, not the model — the model only cites admitted IDs — and claude's independent analysis of the same path (#20) shows everything this batch emits carries both the correlation marker and complete: False, so the confirm path is unreachable for these producers. Converting to a positive binding-vocabulary allowlist is the right hardening, but there is no demonstrated failure trigger in the resulting code. The two panels looked at the same code and reached opposite emphases; neither supports MAJOR.

MINOR (codex MAJOR, chair-downgraded after BASE verification): EC2 NotOptimized as a downsize signal. I verified finops/rules.py:352 already treats Overprovisioned|NotOptimized as the EC2 cost-saving set (with an explicit comment excluding Underprovisioned/Optimized) and rules.py:399 restricts RDS to Overprovisioned — this batch's _DOWNSIZE_FINDINGS is exact parity with established repo logic, as its comment claims, and _sizing_supported layers full measured-pressure requirements on top. Residual risk that a NotOptimized row is directionally ambiguous is real but inherited, not introduced; tightening (positive-savings or recommended-vs-current type comparison) is a good follow-up.

MINOR (claude): the observed_network_flow record inherits binding: "dns_window_correlation" and incomplete coverage from the DNS facts, so NFM 5-tuple matches can never confirm an observation — pin down whether that is intended before test_observations.py locks it in; posture_counts is a capped ACTIVE+NEW sample that diverges from sources.collect_posture's ACTIVE-only rollup (two posture numbers for one account in one report); _network_matches never unconditionally checks the flow row's own account id, unlike _dns_binding; _dns keys configs by a possibly-None Id, and one start_query ClientError abandons the remaining groups in that region.

Positive (claude): the completeness accounting in supporting.py (single-region refusal via regions_not_queried, resource_cost: False, mixed_currency) is correct and the strongest part of the batch.

L5 — Docs / ADR consistency

Cross-batch dependency, not gated here (downgraded from claude's MAJOR): five new DIAGNOSIS_* env flags, the 35 s stage deadline, the member-read path and a large reason-code vocabulary need operator documentation. docs/reference/06-workers.md is changed in another batch and plausibly covers it; docs/guides/diagnosis-evaluation.md — the canonical home of DIAGNOSIS_* behavior — is in neither this batch nor the PR-wide path list. The batch reviewing 06-workers.md must confirm coverage; if it doesn't cover the flag surface, the gap becomes blocking there.

MINOR (claude): member_external_id_required is stricter than ADR-011 as amended (…externalid_optional.sql permits first-party rows without an ExternalId, and sg_rule_scan.py/network_path.py honor that) — the fail-closed divergence is deliberate per the inline comment but should be recorded in docs, not only in code; the inline IAM Sid cross-references are excellent practice but five of seven Sids resolve only in batch 1 and will rot unless ci_runtime_policy.py/runtime_iam.tftest.hcl assert the mapping mechanically.

Verified-accurate parity claims (claude, chair spot-checked): finops/rules.py downsize sets, sources.collect_posture fallback/InvalidAccessException handling, sg_rule_scan 3600 s duration, and test_budget.py's cross-module budget/env assertions against BASE report.py all check out. CHANGELOG rules not implicated in this batch.

3. Suggestions

1. Add db_instance_class to _TYPE_KEYS and add an IPv6 rule to the redaction path (or mask answer_ips explicitly in evidence.py) — both are small, local fixes.
2. In batch 1, confirm: logs:StartQuery resource-scoped to the allowlisted log groups; Stop* actions usable post-deadline; the diagnosis-network/evidence statements attached to both worker roles; DIAGNOSIS_HOST_ACCOUNT_ID actually wired (or reuse AWS_ACCOUNT_ID).
3. Shift the observation window back by an ingestion-lag allowance and validate empirically that NFM/Resolver reads return rows.
4. Decide and document whether NFM flow evidence is ever meant to confirm an observation claim; if yes, stop copying the DNS binding marker into flow facts.
5. Drop the redundant Unit filter from the memory MetricStat; align validate_request kinds with _sizing_supported; make _admit_relationships use _RELATIONSHIP_KINDS; surface the posture filter in admitted facts.
6. Document the new DIAGNOSIS_* flags and the ExternalId-mandatory divergence from ADR-011 in 06-workers.md/diagnosis-evaluation.md.

4. Verdict

Blocking: one chair-verified L2 MAJOR (db_instance_class → <host> corruption), one L3 MAJOR with a chair-verified premise (IPv6 reaches the model; BASE redaction is IPv4-only; security severity not reducible by the cross-batch pointer), one L3 MAJOR IAM-scoping reconciliation owed to batch 1, and one L4 MAJOR (window placement likely renders the observation feature inert). Codex's two L4 MAJORs were downgraded after verification against the diff and BASE. No image evidence exists for this PR.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 4/5 — PASS

Verified: BASE's scripts/v2/workers/diagnosis/ contains neither discovery.py nor evidence.py nor workload_identities.py nor memory_metrics.py — all are listed in the other-batch changed paths, so they are plausible cross-batch dependencies, not missing symbols. The fixture defect both panels flagged (inventory_sync_runs counted from self.rows instead of the scope-filtered scoped) is confirmed directly in the diff at Database.run. The web producer contracts the tests mirror (web/lib/graph-state.ts, web/lib/trace-graph.ts) exist in BASE. The manifest lists zero images, no unavailable or omitted entries.

Chair Review — PR #246, batch 4/5 (feat: add default-off diagnosis discovery and evidence core)

1. Summary

This batch adds three new offline test modules (test_discovery.py, test_evidence.py, test_inventory_identities.py) for the diagnosis discovery/evidence core; no production code, IAM, Terraform or docs change in this batch. Every production symbol the tests import (diagnosis.discovery, diagnosis.evidence, diagnosis.workload_identities, diagnosis.memory_metrics) is absent from BASE but supplied by explicitly listed other-batch paths (scripts/v2/workers/diagnosis/{discovery,evidence,workload_identities,memory_metrics}.py) — an unverified cross-batch dependency, not a missing definition. Both panels independently converged on the same single confirmed fixture defect and found no CRITICAL/MAJOR issues.

2. Issues per lens

L2 — Code correctness

• MINOR (both models flagged — confirmed in diff): test_discovery.py:53 — the Database fake's inventory_sync_runs branch computes row counts from unfiltered self.rows (len([r for r in self.rows if r[2] == kind])) while the inventory_snapshots branch counts the scope-filtered scoped rows. In test_member_reads_only_its_inventory_and_configuration_graph this yields a ledger/snapshot count mismatch (2 vs 1), which per the suite's own ledger_count fault case is the inventory_unqualified condition — so the member test silently exercises an unqualified inventory state rather than a healthy one. Filter the ledger branch by scope. Independent agreement plus direct diff confirmation make this solid, but it is a test-fixture fidelity gap, not a production bug.
• MINOR (claude only): brittle positional indexing in test_evidence.py (data["evidence"][2]/[3]/[4], admitted_bundle()["evidence"][2]/[3]) — reordering bundle() silently retargets assertions; prefer lookup by oid(...).
• MINOR (claude only): test_invalid_address_is_not_preserved_as_an_internal_identity accepts both None and "<ip>" for all five invalid inputs, so it cannot catch a regression that changes drop-vs-mask behavior per input.
• MINOR (claude only): assorted test-hygiene nits — precedence-dependent filter and redundant _now patch in test_row_freshness_follows_the_configured_graph_window; double-recorded queries in test_transport_timeout_...'s BrokenDatabase.run; unreachable else self.metrics arm in Provider.get_paginator; missing boundary cases (accepted-at-four follow-up requests; disclosure sentinel after host-identity re-point).
• Cross-batch dependency (not blocking): all imported production symbols, constants (MAX_RESOURCES, MAX_XRAY_RESOURCES, MAX_INVENTORY_RESOURCES, MAX_BYTES) and the implied invariant MAX_INVENTORY_RESOURCES ≤ MAX_RESOURCES − MAX_XRAY_RESOURCES must be satisfied by the listed other-batch discovery.py/evidence.py; verified absent from BASE, plausibly provided there — unverified from this batch.

L3 — Security / AWS mutation safety

Both models agree: no blocking findings. Test-only batch; no IAM grants, secrets, mutation paths or auth changes. The ADR-005 frozen-mutation posture is actively reinforced by the tests: the provider double exposes only read operations; validate_request is pinned to reject execute_sql, arbitrary queries and foreign regions; validate_finding rejects mutating verification strings (aws ec2 terminate-instances) and safe_downsize claims; fail-closed behavior on missing/false DIAGNOSIS_EVIDENCE_LOOP_ENABLED and untrusted DIAGNOSIS_HOST_ACCOUNT_ID is covered; member scope performs zero live AWS reads. "Secret"-looking literals are negative-control placeholders asserted absent from output.

• MINOR (claude only): the DB double dispatches by substring and never asserts the statement is read-only; a positive SELECT/WITH/SHOW-only guard would enforce the read-only guarantee at the fake boundary rather than infer it.
• MINOR (claude only): fixture correlation_id charset is wider than the producer's sha256-hex ids (web/lib/trace-graph.ts), leaving a residual prompt-content surface untested; pinning the hex shape would close it.
• Cross-batch dependency (not blocking here, blocking in batch 1's review): the read-only IAM grants these tests imply (cloudwatch:GetMetricData, xray:GetServiceGraph, Compute Optimizer reads, Cost Explorer) must be provided and scoped by the listed terraform/foundation/workers.tf/variables.tf/tests/runtime_iam.tftest.hcl; least-privilege there is judged in its own batch.

L4 — Observability / data-integration correctness

Both models agree the tests substantively and accurately mirror the real producers: claude's cross-checks against web/lib/graph-state.ts (13-case source qualification, bool-as-int trap, 2**53, capturedAtMs semantics), web/lib/trace-graph.ts (200-pair cap + 201 read sentinel, host normalization, provenance), agent/lambda/inventory_read_mcp.py staleness matrix, and genuine AWS API shapes (Compute Optimizer EC2/RDS field asymmetry, 500-query CloudWatch limit, 100-ARN bound) all hold.

• MINOR (claude only): X-Ray request arguments are discarded by the Pager fake — no test pins the get_service_graph time window or bounds.
• MINOR (claude only): Compute Optimizer nextToken pagination is never exercised; a truncated recommendation set would pass as complete coverage.
• MINOR (claude only): Aurora replica health (AuroraReplicaLag vs provisioned-RDS ReplicaLag) has no positive case where an Aurora reader's role is known.
• MINOR (claude only): inconsistent dimensions fact shape between the EC2 fixtures (list of {Name,Value}) and the RDS sizing test (plain mapping), and no RDS analogue of the fault="dimensions" wrong-identity case — the RDS dimension-binding guard may pass unexercised.
• MINOR (claude only): the EBS "attached but attachments: []" disclosure relies on attachments_complete alone; no claim-level negative test prevents certifying "unattached" from that shape.

L5 — Docs/ADR consistency

Both models agree: no blocking findings. ADR bodies live upstream; against CLAUDE.md's invariants this batch adds no third mutation exception — all collectors are reads, the loop is default-off, and the model-facing capabilities are resource_metrics/provider_sizing/graph_refresh only.

• MINOR (claude only, unverified cross-batch): DIAGNOSIS_EVIDENCE_LOOP_ENABLED and DIAGNOSIS_HOST_ACCOUNT_ID appear nowhere in BASE docs/Terraform; their default-off fail-closed semantics must be documented by the listed docs/reference/06-workers.md and bound in the listed Terraform files.
• MINOR (claude only): docs/runbooks/graph-read-contract.md/source-sync-observability.md name only two consumers of the qualification contract; this batch adds a third (Python diagnosis reader) that should be named so threshold changes stay discoverable.
• MINOR (claude only, plausibly cross-batch): the new claim taxonomy (observation/hypothesis/sizing_candidate), six pillars, and numeric/money-claim rejection are user-visible report semantics not reflected in any docs touched in this batch.

3. Suggestions

1. Fix the inventory_sync_runs scope filter in the Database fake (the one both-panel finding) so the member-scope test exercises a qualified, healthy ledger.
2. Replace positional data["evidence"][n] indexing with oid(...)-keyed lookups in test_evidence.py.
3. Add a read-only statement guard to the DB fake and pin correlation_id to the producer's sha256-hex shape.
4. Capture and assert get_service_graph paginate kwargs (time window/bounds); add a Compute Optimizer nextToken continuation case, an Aurora known-role replica case, and an RDS wrong-dimensions fault case.
5. In the other batches (noted for their reviews, not gated here): document the two new DIAGNOSIS_* env flags in docs/reference/06-workers.md, and add the diagnosis reader as a named consumer in the graph-read-contract runbooks.

4. Verdict

No CRITICAL or MAJOR findings in this batch from either panel or from chair verification. All apparent missing-symbol concerns reconcile as listed cross-batch dependencies per the cross-batch rule, and the only confirmed defect is a MINOR test-fixture scoping bug. The image manifest is empty with no unavailable or omitted entries, and both panel cells declared coverage not required.

IMAGE_COVERAGE: NOT_REQUIRED

Batch 5/5 — PASS

All verifications are done. Key reconciliations: production Lambda inventory kind is "lambda" (base sync_lambda.py:77; lambda_function appears in base only as Steampipe table/Terraform resource names, never as a resource kind); cloudwatch:GetMetricData already exists in base workers.tf:867,937; docs/guides/diagnosis-evaluation.md exists; AWSOPS_DISCOVERY_PG_PORT has no base reference; and the root CLAUDE.md's docs-language rule makes new English-only runbook/reference content required, which overturns one panel finding.

PR #246 — Chair Review (batch 5/5): diagnosis discovery and evidence core

IMAGE_COVERAGE: NOT_REQUIRED

The manifest lists no images, no deleted image paths, and no unavailable/omitted entries; both panel cells also declared NOT_REQUIRED. No image evidence is required for this PR.

1. Summary

This batch contains one non-test module (scripts/v2/workers/diagnosis/workload_identities.py — exact, read-only RDS proxy/cluster identity projection), six new diagnosis test suites, IPv6/ARN/host additions to test_redact.py, and a new "Diagnosis evidence core" section in docs/reference/06-workers.md. Both panels agree the batch introduces no AWS mutation, no IAM widening, and no credential leakage, and that the fail-closed scoping design (member STS verification, VPC/ARN exact-match binding, injection-guarded Logs Insights queries) is sound. The gate-relevant question was a single Claude-flagged MAJOR about a test fixture using a resource kind the pipeline never produces; it is real on its premise but its impact is decidable only in the batch that contains observations.py, so it is recorded as an unverified cross-batch dependency rather than a confirmed blocker.

2. Issues per lens

L2 — Code correctness

MAJOR (unverified cross-batch — excluded from this batch's gate; must be confirmed in the batch containing scripts/v2/workers/diagnosis/observations.py) — Lambda shared-ENI guard tested with a kind the pipeline never produces (Claude only; Codex did not flag). test_observations.py::test_node_metadata_and_shared_lambda_addresses_keep_explicit_identity_gaps sets kind = "lambda_function" and asserts lambda_eni_identity_unresolved. I verified the premise against BASE: the inventory/production kind is "lambda" (scripts/v2/steampipe/sync_lambda.py:77), nothing in BASE uses "lambda_function" as a resource kind, and this batch's own test_selection.py / test_selection_postgres.py assert kind "lambda" survives real discovery. If observations.py discriminates on the literal "lambda_function", the test passes while the production guard is dead code and a shared Lambda-ENI DNS source could be mis-attributed to a single workload; if it matches "lambda" or any non-EC2 kind generically, there is no defect and only the fixture value is misleading. observations.py is in another batch, so no concrete failure trigger in the resulting code can be cited from here. The reviewer of that batch must check the discriminator; the fixture should use "lambda" either way.

MINOR (Claude only; verified against the diff) — project_tasks fails open on an absent attachments_complete key (is False check only), unlike every other fail-closed cardinality guard in the module. Whether the producer always sets the key is an unverified cross-batch dependency on discovery.py/sync_lambda.py.

MINOR (Claude only; verified) — the qualified evidence comprehensions in collect_proxies/project_tasks index e["fresh"]/e["coverage"]/captured_at outside the try block, so a malformed evidence row from another collector raises out of the function instead of being disclosed as degraded, contradicting the module's own "projection defect → degraded" contract.

MINOR (Claude only; verified) — the trailing proxy_cap mark (len(...) > 4) is unreachable when the budget runs out mid-loop (return inside the proxy loop), so a deadline-truncated >4-proxy region loses the truncation disclosure.

MINOR (Claude only) — test_selection.py seeds rds_proxy as an inventory row, but that kind is only synthesized live by collect_proxies; the fairness assertion is vacuous for it. Plus test hygiene: monkeypatching the shared stdlib time.monotonic, and unused imports (Database/inventory in test_memory_metrics.py, dt in test_selection_postgres.py). No lint gate exists (per CLAUDE.md), so cosmetic.

L3 — Security / AWS mutation safety

No CRITICAL or MAJOR. Both panels independently agree ADR-005's mutation freeze is respected: the only non-test module performs exactly three read-only RDS describe operations through the injected client_factory, with no boto3.client construction of its own; account/region/VPC containment is structural (re.fullmatch with re.escaped account/region, exact-ARN + VPC + unique-match binding, out-of-scope proxies skipped rather than reported). Test credentials are obvious placeholders. The test suites encode strong negative guarantees (no STS call for unregistered/allowlist-missing/ExternalId-missing members; assumed-principal account verification before any client; Logs Insights VPC-ID injection guard; srcport/@message excluded from queries). The NFM/Logs StartQuery/StopQuery action names are write-shaped but create only ephemeral analysis queries — I concur with both panels that this is not an ADR-005 breach, and the doc states it explicitly.

MINOR (Claude only; verified) — collect_proxies flattens ScopeError into the generic proxy_configuration_unavailable gap, hiding a credential-scope refusal (e.g. member_external_id_required) behind a "provider error" marker. Distinct markers would aid operators; no security boundary is weakened.

MINOR (Claude only) — customer private-zone DNS names (facts["query_name"]) reach the model context unredacted, while the same PR broadens redaction for IPs/AWS endpoints. Appears intentional (resolution evidence is useless without the name) but is an undocumented disclosure surface — worth one sentence in the reference doc.

MINOR (Claude only; defense-in-depth) — regions flow from inventory data into client_factory and ARN f-strings; the IAM region condition backstops this, but an explicit check against coverage["regions"]["selected"] would close it. Upstream validation in discovery.py is an unverified cross-batch dependency.

L4 — Observability / data-integration correctness

Verified contracts that hold: graph node meta keys match web/lib/infra-topology.ts:48-49 exactly (invType/resourceId/inventoryAccount/region); the hand-written temp tables mirror the real topology/inventory migrations; mixed PascalCase/snake_case ECS attachment parsing matches the documented jsonb shape; memory qualification correctly refuses ambiguous/foreign/partial series and degrades provider-only memory signals to hypothesis instead of substituting them for measurements.

MINOR (Codex only; verified against the diff) — collect_proxies applies the [:4] proxy cap before the in-scope VPC/account filter, so four unrelated proxies ahead of the relevant one in the DescribeDBProxies page cause the dependency to be missed even though it was already fetched. The omission is at least disclosed via proxy_cap. Filtering before slicing would fix it.

MINOR (Claude only; verified) — test_selection_postgres.py hand-copies schema DDL instead of reusing the existing test_db.py migration-applying fixture; accurate today, drifts on the next migration, and loses real constraints.

MINOR (Claude only; verified) — proxy-target evidence coverage.complete reflects only provider pagination (Marker), not the shared 16-call cap; rows emitted before a _ProxyBound can claim complete: True while the enumeration was truncated, and EvidenceIndex gates on per-evidence coverage.

MINOR (Claude only; verified: no base reference exists) — AWSOPS_DISCOVERY_PG_PORT is framed as "still accepted" legacy but appears nowhere else in the repo.

The Claude MAJOR above is also an L4 concern (workload attribution correctness) — tracked once under L2.

L5 — Docs/ADR consistency

MINOR (both panels converged independently — verified) — the new 06-workers.md section's module enumeration omits observations.py and supporting.py, the two modules exercising the most sensitive reads (Logs Insights, NFM, Cost Explorer, Security Hub, CloudTrail). Codex additionally noted cloudwatch:GetMetricData; I verified it already exists in base workers.tf:867,937, so the table listing only ListMetrics as the new grant is defensible, but a cross-reference would help auditors.

MINOR (Claude only; verified against the diff) — the doc says both paths fail closed with untrusted_host_scope, but test_supporting.py asserts the supporting path records supporting_host_unknown; and member_external_id_required is documented as a gap while it surfaces as a ScopeError that collect_proxies flattens to proxy_configuration_unavailable. Marker names in the doc should match the emitted ones.

MINOR (Claude only) — "the DIAGNOSIS_EVIDENCE_LOOP_ENABLED check inside discovery.collect()/follow_up() is the only gate" undersells the layering; observations.collect independently honors the same gate plus DIAGNOSIS_NETWORK_EVIDENCE_ENABLED (proven by this batch's own tests).

REJECTED — Claude's "English-only section in a bilingual document": root CLAUDE.md explicitly requires new/rewritten developer content under docs/, including runbooks/references, to be English-only; the new section is compliant and existing bilingual bodies are a migration backlog, not an authoring requirement. Also rejected as a defect: "ADR-005 has no decision record" — CLAUDE.md states ADR bodies 001–021 live in the private upstream repo and are cited here by number only; the absence is the documented convention (though the point that the "query-start actions are not mutations" rationale is load-bearing is a fair observation for the private ADR owners).

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