fix(INE-61): bot-review hardening on Task 129 normalization carrier#15
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR hardens the v4 normalization scaffold by enforcing stricter data shape validation, adding type annotations to digest builder functions, and improving error handling for malformed input files. Schema contracts now require normalization keys and constrain ChangesNormalization Validation Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…rrier Hardens the v4 normalization block against malformed input and tightens the contract surface that CodeRabbit + Codex flagged on PR #10. - contract_test: guard `digest_inventory_findings/3` against non-map digests so `Map.keys/1` no longer crashes the run on a scalar - contract_test: validate `_unresolved_reason` in `stub_value_findings/3` (must be null or the sentinel `"not_yet_derived"`) - contract_test: drop nil-clauses in `digest_findings/2` and `stub_record_findings/3` so explicit `"key": null` produces a finding instead of silently passing - contract_test: distinguish `:missing_input` from `:invalid_json` / malformed-shape in `load_parse_methods_inventory/1` — corrupt JSON or wrong top-level shape now raises with regen instructions instead of silently disabling `parse_methods_digest_covers_inventory` - normalization: add @SPEC on six private helpers per the every-function-gets-a-spec mandate - schema: `validate_v4/1` now descends into `/normalization` so a caller-provided `%{}` fails preflight - exchange_v4.json: tighten `_unresolved_reason` from open-string to enum `["not_yet_derived"]` (closed-vocabulary scaffold sentinel)
c9d0ddd to
1583597
Compare
There was a problem hiding this comment.
Pull request overview
Hardens the v4 “normalization” carrier introduced in Task 129 by tightening schema + preflight validation, adding a dedicated normalization builder, and expanding contract/integration tests to prevent silent drift or crashes on malformed inputs.
Changes:
- Introduces
CcxtExtract.Normalizationto build a compactparse_methods_digestand scaffoldfield_maps/response_envelopes, and wires it into the v4 pipeline/schema path. - Extends v4 preflight validation (
Schema.validate_v4/1) and the v4 JSON schema to require and validate thenormalizationblock shape (including_unresolved_reasonenum tightening). - Adds/updates contract, pipeline, and cached integration tests to assert presence, shape, provenance pointers, and digest coverage behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/integration/cached/schema_v4_emit_cached_test.exs | Asserts v4 cached emit includes populated normalization scaffold and sentinel values. |
| test/ccxt_extract/pipeline_test.exs | Updates v4 expectations around normalization, adds digest projection + provenance assertions, and confirms v3 output omits normalization. |
| test/ccxt_extract/normalization_test.exs | New unit tests for digest projection, scaffold shape, and defensive behavior. |
| test/ccxt_extract/contract_test_test.exs | Adds coverage for new normalization invariants and parse_methods digest inventory coverage behavior. |
| priv/schema/exchange_v4.json | Defines strict Normalization schema and related $defs, including _unresolved_reason enum. |
| lib/ccxt_extract/schema.ex | Emits normalization in v4 and validates required normalization keys in validate_v4/1. |
| lib/ccxt_extract/provenance.ex | Marks normalization pointers as derived in default provenance. |
| lib/ccxt_extract/pipeline.ex | Builds v4 normalization from discovery data and threads it into v4 schema build opts. |
| lib/ccxt_extract/normalization.ex | New module implementing normalization block derivation and helper APIs for invariants/tests. |
| lib/ccxt_extract/contract_test.ex | Adds normalization shape invariant, digest coverage invariant, and parse_methods inventory loader. |
Comments suppressed due to low confidence (1)
lib/ccxt_extract/normalization.ex:66
build/2is implemented defensively (it handles non-map entries viadigest_from_entry/1) and your tests callNormalization.build("not a map"), but the spec only allowsmap() | nil. This mismatch can lead to Dialyzer warnings and misleads callers about accepted input. Consider widening the spec toterm()(or adding a guard that enforcesmap() | niland updating tests accordingly).
@spec build(map() | nil, keyword()) :: map()
def build(parse_methods_entry, _opts \\ []) do
%{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {:ok, %{"exchanges" => entries}} when is_list(entries) -> | ||
| Map.new(entries, fn entry -> | ||
| methods = Map.get(entry, "parse_methods") || %{} | ||
| {entry["id"], Map.keys(methods)} | ||
| end) |
💡 Codex Reviewccxt_extract/lib/ccxt_extract/contract_test.ex Line 1447 in c9d0ddd When a v4 artifact has a malformed non-map ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Follow-up to #10 (Task 129) — addresses the 7 actionable findings CodeRabbit + Codex flagged on the original PR before it merged. All hardening, no behavior change on valid input.
contract_test: guarddigest_inventory_findings/3against non-map digests soMap.keys/1no longer crashes the run on a scalarcontract_test: validate_unresolved_reasoninstub_value_findings/3(must be null or the sentinel"not_yet_derived")contract_test: drop nil-clauses indigest_findings/2andstub_record_findings/3so explicit"key": nullproduces a finding instead of silently passingcontract_test: distinguish:missing_inputfrom:invalid_json/ malformed-shape inload_parse_methods_inventory/1— corrupt JSON or wrong top-level shape now raises with regen instructions instead of silently disablingparse_methods_digest_covers_inventorynormalization: add@specon six private helpers per the every-function-gets-a-spec mandateschema:validate_v4/1now descends into/normalizationso a caller-provided%{}fails preflightexchange_v4.json: tighten_unresolved_reasonfrom open-string to enum["not_yet_derived"](closed-vocabulary scaffold sentinel)Test plan
mix compile --warnings-as-errors— cleanmix test.json— 173/173 pass on the four touched modules' test files (normalization_test, contract_test_test, schema_test, pipeline_test)mix credo --strict --format json— zero new findings on touched files (only pre-existing TODO(Task NN) annotations)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Schema Updates