@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, and scope drift in the milestone issue split/audit routing. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality, CI, WSDL audit integrity, or milestone tracking correctness.

Claude finished @axisrow's task in 5m 8s —— View job

Code Review — Milestone 18 audit routing

Focused on bugs, audit integrity, and milestone-tracking correctness, per request.

Verified clean

• tests/WSDL_OPTIONAL_FIELD_AUDIT.md no longer contains any link to the parent epics (#240, #244, #247, #249, #250, #251, #252, #255) — grep -c returns 0 across both #NNN text and /issues/NNN URL forms.
• Outside the audit report, the only remaining mention of those parent IDs in tests//scripts//direct_cli/ is the new guard literal at tests/test_wsdl_parity_gate.py:1670, which is correct.
• The classification order in scripts/build_wsdl_optional_field_audit.py:108-152 is internally consistent: AUDIT (longest prefix) → CLI options → minOccurs≥1 & depth==1 → INTERNAL_VALIDATION → new CHILD_PREFIX_FOLLOWUPS (longest prefix) → DEFAULT_FOLLOWUPS. The new step sits before defaults so child-subtype prefixes correctly override the per-(group, op) fallback, and after the parity-gate short-circuits so required depth-1 fields stay supported.
• The new dict-driven prefixes mirror the existing AUDIT longest-prefix lookup pattern, so semantics match a reader's expectation.
• Issue-number distribution (#264, #267–#288, #290–#296, #299–#308) is consistent with the PR's "child issues ads update: refactor subtype payload builders and add DYNAMIC_TEXT_AD #267-clients update: ErirAttributes.Contragent typed flags #308" claim plus the pre-existing #254/#256/#264 routes that were intentionally not changed (retargeting/bidmodifiers/FileFeed).

Findings

1. Audit-integrity gap: hardcoded #297/#298 are not covered by the new parent-epic guard. [Medium]
scripts/build_wsdl_optional_field_audit.py:84-104 builds two follow-up entries inline (Strategy CustomPeriodBudget → #297, ExplorationBudget → #298) instead of putting them in OPTIONAL_FIELD_CHILD_PREFIX_FOLLOWUPS. The new test_optional_field_followups_do_not_route_to_parent_epics (test_wsdl_parity_gate.py:1668) iterates the three registry dicts but never imports/inspects the script, so if someone edits those literals to a parent epic ID, the test will pass and the regression will only surface as drift in the rendered audit Markdown. Two mitigations are equally fine: (a) lift the two entries into OPTIONAL_FIELD_CHILD_PREFIX_FOLLOWUPS (and keep the "anywhere in path" matching in the script if you still need substring-style routing for these), or (b) extend the new guard test to also assert no parent-epic strings exist in scripts/build_wsdl_optional_field_audit.py. Fix this →

2. Validation gap on the new registry. [Low]
test_optional_field_default_followups_reference_issues (parity_gate.py:1651) verifies OPTIONAL_FIELD_DEFAULT_FOLLOWUPS keys are in COMMAND_WSDL_MAP and issue looks like #NNN. The new OPTIONAL_FIELD_CHILD_PREFIX_FOLLOWUPS has no equivalent check, so a typo in either the (cli_group, cli_op, prefix) tuple or the issue: key would silently fall through to the default followup without failing CI. Consider extending the existing test (or the new guard) to also walk this dict.

3. Strategies fallback uses parts.index(name) first-occurrence. [Low / informational]
_child_followup_for_path lines 86 and 96 use parts.index("CustomPeriodBudget") and parts.index("ExplorationBudget"), returning the first occurrence. Fine for the current WSDL (these names don't repeat), but worth knowing if the schema ever nests a same-named container — pairs with finding #1, since moving to the registry would force this to be an explicit (group, op, full-prefix) key.

Scope / correctness checks (no issues)

• bidmodifiers (bidmodifiers add: complete optional adjustment WSDL parity #254) and retargeting (retargeting lists: complete optional Description WSDL parity #256) were intentionally not rerouted because they were never on a milestone-18 parent epic; the diff preserves that.
• feeds.add/update re-routes from the old parent #253 to #264 (the FileFeed follow-up), matching the PR description's "added feeds add/update: add FileFeed typed upload support #264 to milestone 0.3.12 as the FileFeed follow-up."
• The new guard tuple {"#240", "#244", "#247", "#249", "#250", "#251", "#252", "#255"} matches the PR's enumerated parent epics exactly; #239 (the umbrella issue) is intentionally not in this set since it's the audit's tracking ticket, not an actionable parent.
• Per CLAUDE.md's strict-WSDL-parity rule, none of the relabeling rewords any rationale into the banned "helper/legacy/not part of strict WSDL parity" phrasing — all notes stay in the "needs typed support or N/A" form, so the rationale guard (test_dry_run_exclusions_have_no_helper_or_legacy_rationale) remains satisfied.

Not run locally

pip install -e ".[dev]" and python3 -m pytest tests/test_wsdl_parity_gate.py weren't executed (sandboxed env without those approvals). Findings above are from static review only; CI should be the authoritative gate.

• Read changed files
• Verify child-issue routing logic correctness
• Check audit report integrity (no parent epic links remaining)
• Static analysis of parity gate + script
• Post review
  · Branch: codex/milestone18-issue-split-audit

Codex Review: Didn't find any major issues. Can't wait for the next one!

Addressed the review feedback in 6f0cbea: moved strategy CustomPeriodBudget/ExplorationBudget routing into shared child follow-up registries and added parity-gate validation for child prefix/component routes. @codex @claude review. Please focus only on blocking/actionable issues that could affect WSDL audit integrity, CI, or milestone tracking correctness.

Claude finished @axisrow's task in 2m 39s —— View job

Code Review — Milestone 18 audit routing (round 2, commit 6f0cbea)

Focused only on blocking/actionable issues affecting WSDL audit integrity, CI, or milestone tracking correctness, per request.

Verified clean

• Strategy entries actually moved. scripts/build_wsdl_optional_field_audit.py:73-92 no longer hardcodes the #297/#298 routes; they now live in OPTIONAL_FIELD_CHILD_COMPONENT_FOLLOWUPS (tests/test_wsdl_parity_gate.py:1312-1333) alongside ("strategies", "update", ...) entries that were missing from the old inline form — so strategies update is now routed identically to strategies add instead of falling through to the per-command default. That is a real correctness improvement, not just a refactor.
• Parent-epic guard widened. test_optional_field_followups_do_not_route_to_parent_epics (tests/test_wsdl_parity_gate.py:1751-1753) now also walks OPTIONAL_FIELD_CHILD_COMPONENT_FOLLOWUPS, closing the gap I flagged in round 1. With the inline literals gone, the parent-epic regression I worried about can no longer slip through.
• New validator covers the prefix and component dicts. test_optional_field_child_followups_reference_real_paths (tests/test_wsdl_parity_gate.py:1687-1731) asserts, for both dicts: (a) the command key exists in COMMAND_WSDL_MAP, (b) status ∈ OPTIONAL_FIELD_AUDIT_STATUSES, (c) issue matches #NNN, and (d) the prefix/component actually appears in the audited WSDL paths. This addresses three of the four sub-points Copilot raised on the prefix dict.
• Classification order still sound. _classify_row in scripts/build_wsdl_optional_field_audit.py:95-139 resolves OPTIONAL_FIELD_CLI_OPTIONS and the minOccurs>=1 / depth==1 parity gate before consulting the new child registries, so the registries cannot mask newly supported paths or required depth-1 fields at the row level. Copilot's masking concern (sub-point d) is therefore inapplicable to prefix/component followups by construction — they're only consulted as a fallback, not as overrides like OPTIONAL_FIELD_AUDIT.
• Component lookup is index-safe. The parts.index(component) inside the for component in parts loop (scripts/build_wsdl_optional_field_audit.py:85-91) always returns the same leftmost index the iterator is at, so matched_path is well-defined even if a name were to repeat in a future schema.

Findings

No blocking findings. All round-1 issues that could affect audit integrity or milestone tracking are resolved.

Non-blocking observations (informational only — no action required)

• OPTIONAL_FIELD_AUDIT_STATUSES accepts supported / not_applicable / missing_followup, and the new test allows any of the three on followup entries. In practice every entry in both child dicts is missing_followup, so the looser check is fine; tightening it would be a style call, not a correctness gap.
• Copilot's "prefix doesn't overlap OPTIONAL_FIELD_CLI_OPTIONS" suggestion is unnecessary given the resolution order — CLI options win at the row level, and a registry entry at Foo legitimately covers deeper rows like Foo.Bar that have no typed support yet. Adding the overlap check would risk false positives.

Not run locally

pytest tests/test_wsdl_parity_gate.py and python3 scripts/build_wsdl_optional_field_audit.py --check weren't executed here — review is static. CI is the authoritative gate.

• Read the diff for 6f0cbea (script + parity gate)
• Verify the strategy CustomPeriodBudget/ExplorationBudget routes were actually moved into the shared registries
• Verify the new parity-gate validators cover prefix/component routes (well-formed #NNN, no parent epics, no masking of supported paths)
• Re-check the audit Markdown for any remaining parent-epic links (confirmed clean in round 1; unchanged here)
• Post findings

Verdict: LGTM from an audit-integrity / CI / milestone-tracking standpoint. The round-1 audit-integrity gap (hardcoded #297/#298 outside the parent-epic guard) is properly closed, and the new validator prevents future drift.
· Branch: codex/milestone18-issue-split-audit

Codex Review: Didn't find any major issues. What shall we delve into next?

Review triage after 6f0cbea: Copilot inline comments 3293993999 and 3293994010 are FIXED by moving strategy budget routes into OPTION_FIELD child component registry and adding child prefix/component route validation. Claude round 2 reports LGTM/no blocking findings; Codex reports no major issues. CI is green on the latest SHA.