[ores.trading] Port Batch 2 trading entity-convention rollout

[mcraveiro]

Summary

Slice of the batch-2 cross-component badge rollout, restricted to trading: migrate the party_role_type and trade_type models to the full entity shape (primary key folded into Columns, Table display section) and port the generated table, eventing, generator, messaging, presentation, repository, and service files that are absent on main. The source commit's other components (iam, reporting, dq, qt, sql, codegen) are excluded: their content is superseded on main or deleted surface. The regenerated generators follow the current codegen convention (tenant-scoped generation, thread-safe counter, version=0); their tests were updated to match — the same stale-expectation fix already applied to the refdata generators. Closes task 08547F5A (batches 1-2 shipped); batch 3 (plain-text documentation) is captured.

Changes

• Migrate party_role_type and trade_type models to the full entity shape: primary key folded into Columns, Table display section
• Port generated table, eventing, messaging, presentation, repository, and service files absent on main
• Regenerated generators: tenant-scoped generation (generation_keys::tenant_id), thread-safe counter, version=0 per the current codegen template
• Update generator tests to the current codegen version==0 convention (stale-expectation fix mirroring refdata)
• Close task 08547F5A — batches 1-2 shipped; batch 3 tracked as capture https://orestudio.github.io/OreStudio/doc/agile/product_backlog/inbox/batch_3_of_the_badge_image_plain_text_rendering_ro.html
• Verification: local build clean (linux-clang-debug-make); rat full suite passed — 0 failures across all suites (trading api 419 assertions / 98 test cases)

Traceability

Artefact	Link	ID
Story	Roll out badge/image/plain-text rendering across classified domain entities	6AC16489-70F6-4F95-A5FA-0D561FF151A3
Task	Implement classified entities in batches	08547F5A-B07C-4429-9EA5-022ACDC882C9
Environment	solid_dirac

🤖 Generated with Claude Code

[mcraveiro]

@claude please review this pull request

[claude]

Claude finished @mcraveiro's task in 7m 0s —— View job

---

Code Review — PR #1950

• Gather context (diff origin/main...HEAD, 59 files, ~1860 insertions)
• Run code review
• Post findings

I reviewed the full diff and checked the new messaging/eventing wiring against actual call sites, and compared the new generators against the batch-1 reference pattern in ores.refdata.

Findings (most severe first)

1. party_role_type messaging stack is never registered — projects/ores.trading/core/src/messaging/party_role_type_registrar.cpp:34
register_party_role_type_handlers() has zero callers in the repo. No NATS subject (list/save/delete/history) is subscribed, so the whole feature this PR adds (repository, service, handler, protocol) is unreachable over the wire.
   Fix this →

2. Event mapping registrars never invoked — projects/ores.trading/service/src/app/application.cpp:136
register_party_role_type_event_mapping() and register_trade_type_event_mapping() are added in this PR but not called. application.cpp wires event mappings for six instrument types (lines 119-136) but omits both new entities, so the Postgres → NATS change-event pipeline stays silent for them even though DB writes succeed.
   Fix this →

3. New trade_type_handler/trade_type_registrar are dead code — projects/ores.trading/core/include/ores.trading.core/messaging/trade_type_handler.hpp:1
registrar_trades.cpp still routes trade_type CRUD through the pre-existing instrument_ref_handler (see its own comment: "trade types remain handled here via instrument_ref_handler"). The new handler/registrar compile and link but nothing calls them, so any future fix applied only to the new handler has no real effect.

4. Silent reply drop on decode failure — projects/ores.trading/core/include/ores.trading.core/messaging/instrument_ref_handler.hpp:129
delete_impl and history_impl (touched by this PR) don't call error_reply() when decode<Req> fails, unlike the sibling party_role_type_handler/trade_type_handler. Since instrument_ref_handler is the live path for trade_type, a malformed delete_trade_type_request or history request leaves the NATS requester waiting for its own timeout instead of getting an immediate error.

5. Generators double-increment the uniqueness counter — projects/ores.trading/api/src/generators/party_role_type_generator.cpp:44 and .../trade_type_generator.cpp:43
   Each generator calls counter.fetch_add(1) to get idx, then also applies a leftover ++counter in the same code string — a stale expression from the old (pre-migration) modeling doc. Compare with ores.refdata's calendar_type_generator.cpp, which uses idx alone. Result: two atomic increments per generated instance, and non-monotonic, confusing generated codes. Root cause is in the modeling docs themselves — projects/ores.trading/modeling/ores.trading.party_role_type.org:41 and ores.trading.trade_type.org:41 still specify std::to_string(++counter), so regenerating from these docs reproduces the bug.

6. trade_type table dump loses the Product column — projects/ores.trading/api/src/domain/trade_type_table.cpp:31
convert_to_table() drops the product_type ("Product") column entirely rather than just reformatting it. A user inspecting trade types (e.g. via CLI table output) can no longer see which product family (swap/fx/bond/…) a trade type belongs to.

Not blocking, but worth a look

None beyond the above — the codegen-convention updates (tenant-scoped generation, version=0) and the migration of both models to the full entity shape (primary key folded into Columns, Table display section) otherwise look consistent with the batch-1 rollout pattern.

[claude]

Claude finished @mcraveiro's task in 3m 22s —— View job

---

Code review in progress

• Gather context (PR description, changed files)
• Run /code-review skill against PR [ores.trading] Port Batch 2 trading entity-convention rollout #1950
• Post review findings

[claude]

Claude finished @mcraveiro's task in 2m 11s —— View job

---

Review complete

• Orient with compass.sh bearings
• Gather context (diff, changed files)
• Review generated files (table/eventing/messaging/presentation/repository/service) for correctness and consistency
• Review entity-shape migration for party_role_type and trade_type
• Review generator changes (tenant-scoped, thread-safe counter, version=0)
• Post review feedback

Note: Bash tool calls (including compass.sh) required approval that wasn't available in this session, so I reviewed by reading the diff and source files directly rather than building/running tests. Build/test verification claimed in the PR description could not be independently re-run here.

Summary

Solid, well-scoped port. The entity-shape migration (primary key folded into Columns, Table display section) and the generated table/eventing/messaging/presentation/repository/service files for party_role_type and trade_type are consistent with the equivalent, already-merged refdata rollout (e.g. party_type). Repository additions (read_at_version, paginated read_latest, get_total_*_count), registrars, event registrars, and handlers all match the established convention.

Bug: leftover ++counter duplicates the atomic increment in both generators

projects/ores.trading/api/src/generators/trade_type_generator.cpp:42-44 and projects/ores.trading/api/src/generators/party_role_type_generator.cpp:43-45:

const auto idx = counter.fetch_add(1, std::memory_order_relaxed);
r.code = std::string(faker::word::noun()) + "_trade_" + std::to_string(++counter) + "-" +
         std::to_string(idx);

This increments the same atomic counter twice per call: once via fetch_add, again via ++counter. Compare with the already-regenerated party_type_generator.cpp (refdata), which only uses idx:

r.code = std::string(faker::word::noun()) + "_type" + "-" + std::to_string(idx);

The trading .org model sources weren't updated to match either — ores.trading.party_role_type.org and ores.trading.trade_type.org still carry the stale generator snippet std::string(faker::word::noun()) + "_role_" + std::to_string(++counter) (pre-tenant-scoping convention), instead of the refdata pattern (no ++counter, codegen appends -<idx>). It looks like the .cpp generators were hand-patched with the new tenant-scoped idx logic but the redundant ++counter from the old snippet wasn't removed, and the .org source of truth was never updated to match — so a future regeneration from the .org files would reintroduce the same stale pattern.

Effect: not a crash, but codes end up with a confusing double suffix (e.g. noun_role_2-0) and the counter advances by 2 per generated record instead of 1 — inconsistent with the rest of the codebase's generator convention.

Fix this →

Minor / non-blocking

• product_type conversion validation was dropped from trade_type_mapper.cpp (map) — previously it threw std::logic_error on product_type::unknown. This is fine since trade_type_entity::product_type is now a typed domain::product_type column (sqlgen handles the enum directly) rather than a std::string, so the string-parse failure mode no longer applies — just flagging for confirmation this was intentional and not a silent behavior loss.
• <unordered_set> is included but unused in both regenerated generator .cpp files — pre-existing pattern from the codegen template elsewhere in the codebase, so not introduced by this PR; not worth a special fix here.

[mcraveiro]

All six findings fixed.

1-3. Wiring — fixed in f95e305: register_party_role_type_handlers appended to the fan-out in core/src/messaging/registrar.cpp; register_trade_type_handlers now called from registrar_trades.cpp (the four instrument_ref_handler trade-type subscription blocks replaced); application.cpp registers the party_role_type and trade_type event mappings alongside the six instrument mappings, before event_source.start().

4. Moot by retirement — fixed in f95e305: instrument_ref_handler.hpp is deleted (header-only; its only callers were the replaced trade-type blocks). trade_type now routes through the entity-shaped handler stack, which calls error_reply() on decode failure — the silent-drop path is gone.

5. Fixed in 185bb1f: both generators and both org generator expressions dropped the leftover ++counter; codes now follow the refdata party_type pattern (noun_role dash idx), one atomic increment per record.

6. Fixed in 185bb1f: the product_type column is added to the converter and the org Table display section. One note: product_type is the first enum column in an entity table display, and the codegen table template streams enum columns raw — which cannot compile, as no operator-less-than-less-than exists for generated enums. The column is rendered via to_string() instead; the template gap is captured as codegen-table-enum-columns.

Verification: local build clean (linux-clang-debug-make); ctest 71/71 passed, including all three ores.trading suites.

[claude]

Claude finished @mcraveiro's task in 7m 20s —— View job

---

Code review in progress

• Gather context (PR description, changed files, prior review comments)
• Run /code-review skill against PR [ores.trading] Port Batch 2 trading entity-convention rollout #1950 (multi-angle review in progress)
• Post review findings
  — branch feature/badge-batch2-trading

[mcraveiro]

Fixed in 185bb1f — both generators and both org sources dropped the leftover ++counter, matching the refdata party_type pattern.

Minors, not changed:

• product_type validation: intentional, confirmed. trade_type_entity::product_type is a typed domain::product_type column and sqlgen handles the enum directly; the string-parse logic_error failure mode no longer applies.
• unused unordered_set include: pre-existing codegen template pattern across the codebase, not introduced by this PR.

Verification: local build clean (linux-clang-debug-make); ctest 71/71 passed, including all three ores.trading suites.