@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, stale audit links, and false closure of #239. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality, make the optional-field audit misleading, or cause real harm in production.

Claude finished @axisrow's task in 4m 54s —— View job

Claude Code Review — PR #248

Todo

• Read PR diff and changed files
• Examine direct_cli/wsdl_coverage.py recursion changes
• Examine scripts/build_wsdl_optional_field_audit.py
• Examine new test gate in tests/test_wsdl_parity_gate.py
• Examine tests/WSDL_OPTIONAL_FIELD_AUDIT.md snapshot
• Check whether audit: WSDL parity gate skips optional fields — audit all *Update/*Add types for missed CLI flags (CalloutSetting pattern) #239 is correctly closed
• Post final review

Summary

This is a well-scoped, soft-gate audit. No bugs, security issues, data‑loss risks, or production hazards found. The changes are read-only (audit + tests + docs) and cannot affect any mutating CLI path. Below are a few real risks that could make the audit misleading over time, plus one minor non-issue.

Critical-ish findings

1. _camel_to_option heuristic produces false-positive supported rows when leaf names collide. scripts/build_wsdl_optional_field_audit.py:51-54 looks up by leaf field name only, so multiple WSDL paths that share a leaf name map to the same Click option. Example from tests/WSDL_OPTIONAL_FIELD_AUDIT.md:

• adgroups.add DynamicTextFeedAdGroup.FeedId → supported (--feed-id)
• adgroups.add SmartAdGroup.FeedId → supported (--feed-id)
• adgroups.add TextAdGroupFeedParams.FeedId → supported (--feed-id)

In reality there is only one --feed-id Click option on adgroups add, and it's wired into exactly one subtype block. The audit currently asserts all three are covered, which masks any subtype that isn't actually wired. Today this is mitigated by the parent block being flagged missing_followup (#247), but as those follow-ups are closed, the leaf rows will silently flip from "supported (true)" to "supported (false)" without anything noticing. Consider matching on path as well as leaf name in the heuristic, or extending WSDL_FIELD_TO_CLI_OPTION to accept full paths.

2. _issue_link hardcodes the repo slug (scripts/build_wsdl_optional_field_audit.py:57-60):

return f"[{issue}](https://github.com/axisrow/direct-cli/issues/{issue[1:]})"

If the repo is ever renamed/transferred, every missing_followup link in WSDL_OPTIONAL_FIELD_AUDIT.md becomes stale and the regenerated audit will keep emitting dead links — test_optional_field_audit_report_is_current won't catch it because both sides are wrong. Pull the slug from git remote get-url origin or a constant at top of file so it's a single point of failure. Fix this →

3. Soft cache fallback to live WSDL inside the audit gate. fetch_wsdl() at direct_cli/wsdl_coverage.py:164 silently falls back to a network fetch + cache write when the cache file is missing. The new test test_optional_field_audit_report_is_current calls it via subprocess; if a fresh CI checkout (or a future cache cleanup) removes any tests/wsdl_cache/*.xml, the test will go to the network, potentially generate a different audit than the committed Markdown, and either (a) flake on transient network errors or (b) cause a spurious "audit stale" failure that masks the real cause. Not introduced by this PR, but the new soft-gate is the first thing that runs the full container walk, so it's the most likely surface for this. Worth either passing use_cache=True, allow_network=False semantics or asserting cache presence before walking. Fix this →

Minor / non-blocking

4. OPTIONAL_FIELD_AUDIT override precedence is unconditional. In _classify_row (scripts/build_wsdl_optional_field_audit.py:69-74), a manual entry wins even if the field has since been implemented. That's the intended behavior (manual ledger is authoritative), but it does mean once a follow-up is shipped, you have to remember to remove the override or the audit will keep reporting missing_followup for a field that's actually live. The _audited_wsdl_paths invariant only catches stale paths, not stale statuses. Consider a separate test that fails when an override says missing_followup but the heuristic detects a typed CLI flag — that flips the audit into a continuous reminder to close follow-ups.

5. iter_container_item_fields recursion is bounded by max_depth (direct_cli/wsdl_coverage.py:564-619), so no cycle risk even for a self-referential WSDL type — good. Shallow-copy of each field excluding item_fields is correct.

6. Determinism of render_markdown. Iteration order is sorted(COMMAND_WSDL_MAP.items()) then WSDL-XML field order; confirmed filter preserves that order. Reproducible across runs. ✓

7. Hard-gate coverage of nested required fields is still missing, e.g. MobileAppAdGroup.StoreUrl (min=1, depth=2) shows as missing_candidate, not required_hard_gate, because get_required_item_fields only walks depth=1. This PR doesn't regress the hard gate — it just newly exposes those rows in the soft audit — but the message "covered by minOccurs>=1 parity gate" is only emitted for depth=1, which is the right call. No action needed; calling out for the next maintainer reading the table.

#239 closure

The PR description claims Closes #239 and what it actually ships is the audit gate itself plus six follow-up issues for the concrete CLI gaps. That's a legitimate scoping decision — making the gaps visible and tracked is exactly what a "missing optional fields" audit should land. Closure looks accurate given the audit registry covers the 29 confirmed gaps and the soft gate prevents future drift. (I could not directly fetch the #239 body in this sandbox to triple-check phrasing; if the issue body explicitly asks for the missing fields to be implemented rather than catalogued, the PR description should be reworded to "Refs #239" and keep #239 open until #242–#247 ship.)

Things I checked and found no issue with

• No security-sensitive surface introduced (no eval, no shell, no auth, no IO outside repo).
• subprocess.run in test_optional_field_audit_report_is_current uses sys.executable + check=False and inspects returncode — no injection, no hang.
• No mutation paths touched; smoke-matrix categories unchanged.
• WSDL field walk has no cycle risk thanks to max_depth=2.
• The new public function iter_container_item_fields is purely additive and doesn't shadow get_required_item_fields.

· Branch: feat/issue-239-optional-fields-gate