Levandor

BE-3482 pymando Branch Review

13 min read

BE-3482 pymando Branch Review

Read-only review (2026-07-13) of feature/BE-3482-pymando, 13 commits off develop@3024e20c in worktree .worktrees/BE-3482-pymando, judged against the mando AGENTS.md conventions guide. Verdict: NEEDS REWORK BEFORE MR. The conformance work itself is solid and the quality gates are genuinely green, but three blockers are things reviewers in this repo reject on sight, and all three are cheap to fix (about 30 minutes). Update, same day: the rework was EXECUTED as a from-scratch rebuild on feature/BE-3482-pymando-v2; see the “Rework executed” section below.

For Agents

FINAL OUTCOME 2026-07-22: v2 was pushed, opened as MR !570 and MERGED into develop (squash fde2425e “feat: pymando logging conformance”, merge commit 14dfb3ad). The conformant wheel py-mando==1.16.1+dev.2691658359.14dfb3ad is published to Nexus dev and consumed by BE-3613. This note is now historical record of the review + rework. No mutations were made during the review (no commits, amends, rebase, push, or file edits; only build/test/check commands in the target worktree). UPDATE 2026-07-13: the rework was executed the same day. The fingerprint branch went first as planned and is now open as MR !569 (pushed to origin, awaiting review; see BE-3482 Datadog Logs and APM Conformance). This branch was rebuilt from scratch as feature/BE-3482-pymando-v2 (worktree .worktrees/BE-3482-pymando-v2, 15 title-only commits, all gates green incl. pytest 120 passed / 1 skipped, which satisfies the re-run requirement below). v2 is NOT pushed and has no MR; the old v1 branch and worktree are untouched as fallback. Full outcome in the “Rework executed” section.

What the branch contains

Two workstreams bundled into one branch, net +470/-196 across 13 files (2 new: mando-lib/src/tracing/fmt_util.rs, py-mando/test/test_dd_conformance.py).

On-ticket (9 commits): py-mando DataDog log/APM conformance. ECS team-label extraction; logger.init(service, team) building an identity dict and exporting DD_SERVICE/DD_ENV/DD_VERSION; TraceFilter emitting flat flow.step.path and unified ddtags; span-tag rename flow.context to flow.step.path; conformant logging.conf (logger.* fields, status, drops the phantom %(status)s); a log_error helper emitting error.{type,kind,code,message,stack,fingerprint} with root-cause unwinding; the Rust JsonFormatter renamed to status/logger.file/logger.line; a set_log_tags pyfunction bridging ddtags into the Rust stream via LOG_DDTAGS: OnceLock<String>; a 177-line conformance test file (8 tests).

Off-ticket (4 commits): 86d7ffd8 version-fallback panic fix (byte-identical duplicate of bc3727ba on the fingerprint branch, same 4 files, same +39/-8), 34e749d5 inline-format tweak on the exact line 86d7ffd8 rewrote, 9b0c4e70 logger-init split with focused tests, d0986230 extraction of collect_span_fields/MapVisitor/WriteAdaptor/level_to_status into a new ungated mando_lib::tracing::fmt_util.

Gate results (genuinely green)

  • rtk proxy cargo clippy --release --all-features: exit 0, ZERO findings on branch-touched files (all remaining warnings are pre-existing, in untouched crates).
  • rtk proxy cargo test --all-features --release -- --test-threads=1: 366 passed / 0 failed / 35 ignored. Nonzero totals confirm the RTK false green did not occur.
  • Feature-flag correctness proven by BUILD, not assumed: cargo check -p mando_lib --no-default-features passes AND --no-default-features --features python passes, so fmt_util does NOT accidentally require the app feature (tracing-subscriber is an unconditional dep).
  • All 4 new Rust tests confirmed executing in the gate run.
  • Python pytest suite NOT re-run this session; last verified 2026-07-09 (114 passed / 1 skipped). Since logging.conf, logger.py, and tracing.py all changed, this MUST be re-run before MR.

Blockers (3)

  1. Scoped commit titles, 13/13. Every title carries (mando) or (py-mando); AGENTS.md 2.7 bans scopes outright (reviewer-mandated in MR !322). All 13 need retitling.
  2. 7 orphaned ContextVars in py-mando/python/py_mando/tracing.py (declarations at lines 13-19, dead .set()/.reset() at 39-45 and 52-58). Commit d528b71f deleted their only reader (the tags block in TraceFilter.filter); grep confirms zero readers repo-wide. Knock-on: trace()’s metadata param now feeds nothing but dead vars, and run_with_trace still passes _logger.metadata into it. This is the exact theme-2 rejection pattern (“remove the unused code”, MR !345).
  3. Off-ticket duplicated commits. 86d7ffd8 + 34e749d5 are unrelated to the ticket, AND 86d7ffd8 is a byte-identical duplicate of bc3727ba on the fingerprint branch. Whichever MR lands second gets conflicts in all 4 files on identical content.

Notable should-fixes

  • error.fingerprint serializes code|None when no step context is set (tracing.py:109, f"{code}|{context_var.get()}"). Section 6.3 defines the fingerprint as error code plus step path; a missing path should be omitted or empty, not stringified as "None". Pollutes DD fingerprint aggregation for every log_error call made outside a trace()/run_with_trace scope. No test covers that path.
  • Dead tracing-serde dep in py-mando/Cargo.toml:78; fe78dc10 removed the last use. (py-mando-simulation still uses it legitimately; leave the workspace entry alone.)
  • fmt_util extraction only 2/3 done. py-mando-simulation/src/log_formatter.rs still duplicates collect_span_fields/MapVisitor/WriteAdaptor verbatim, and its Cargo.toml already depends on mando_lib with features = ["python"], so mando_lib::tracing::fmt_util is directly reachable today. Theme 5 (finish the pass before requesting review) plus 13.4 (duplicate logic is debt).
  • 2 branch-introduced rustfmt hunks: fmt_util.rs:47 (NEW file, theme 7: rustfmt every new file) and dd_formatter.rs:373 (new test line exceeding struct_lit_width). Hand-fix those two lines only; a full rustfmt of dd_formatter.rs would also rewrite its 6 pre-existing hunks (unrelated churn, theme 1).
  • super::version_or(...) called by full path 3 times in the dd_formatter.rs test module (lines 370, 372, 374) despite use super::*; at line 164. The single most repeated review nit in this repo.
  • Test hygiene: hand-rolled _Capture handler where pytest’s built-in caplog fixture does the job; the 3 log_error tests duplicate the same 4-line setup verbatim and leak handlers onto a module-level logger; test_init_populates_identity_and_dd_env never restores _initialized, the root handlers, or identity/metadata, even though commit 491a293b’s title claims “restore mutated globals” (the restore is not present in the final file).
  • Minor: two use crate::log_formatter::... lines to merge (py-mando/src/lib.rs:65-66); inert # noqa: F401 comment (logger.py:73, py-mando has no linter configured); cross-module use of the private _logger._build_ddtags() sets up a fragile circular import.

Rebase risk and landing order

  • develop tip a01892f9 is 14 commits ahead of the branch base (79 files changed on develop); the intersection with branch-touched files is exactly ONE file: mando-lib/src/app/dd_formatter.rs.
  • The conflict is semantic and guaranteed: develop’s ae6d1098 (the BE-3482 core merge) rewrote dd_formatter.rs inline with the same plumbing d0986230 extracts (collect_span_fields, MapVisitor, WriteAdaptor, its own level-to-status match, logger.file/logger.line). The branch deletes hunks develop rewrote and relocated; git cannot usefully auto-merge. Must be re-done by hand if kept.

Recommended landing order:

  1. Fingerprint MR first (feature/BE-3482-fingerprint-activity): already based on the develop tip, correctly titled, and it unblocks develop’s live mando-bess panic.
  2. Then rebase this branch onto develop, dropping 86d7ffd8 (already on develop via bc3727ba by then; verify the rebase auto-drops it as an identical patch) and 34e749d5 (empty or one-char churn once 86d7ffd8 is gone).
  3. Sharpest option (recommended): drop d0986230 too. It is the entire conflict surface, off-ticket, and only 2/3 finished. Result: a zero-conflict 9-commit on-ticket MR. Refile the fmt_util extraction as its own ticket that unifies all THREE formatters (dd_formatter + py-mando + py-mando-simulation) against develop’s conformant dd_formatter. Cost: fe78dc10 currently takes level_to_status from fmt_util, so py-mando temporarily needs its own local level-to-status match (about 7 lines), which the follow-up ticket then de-dupes. Alternative: keep d0986230, but FINISH it (fold in py-mando-simulation) and accept the manual rebase rework.

No refinery migrations on the branch, so the migration-superset check is N/A.

Shortest path to MR-ready (9 items)

  1. Strip scopes from all 13 commit titles (BLOCKER, ~10 min).
  2. Delete the 7 orphaned ContextVars and their set/reset in trace(); collapse the now-unused metadata param and its run_with_trace call site (BLOCKER, ~10 min).
  3. Drop 86d7ffd8 + 34e749d5; the version fix lands via the fingerprint MR (BLOCKER, ~10 min).
  4. Remove the dead tracing-serde dep (~2 min).
  5. Fix the code|None fingerprint and add the missing no-step-context test (~10 min).
  6. Hand-fix the 2 rustfmt hunks; drop super:: from the 3 version_or calls; merge the two use crate::log_formatter::... lines; delete the # noqa (~10 min).
  7. Decide on d0986230: drop it (recommended, zero-conflict MR) or finish it and hand-rework on rebase.
  8. Re-run maturin develop --release + poetry run pytest --nbval (GATE, ~15 min).
  9. Swap _Capture for caplog; parametrize the 3 duplicated log_error tests; restore mutated globals (~20 min).

Blockers (items 1-3) are roughly 30 minutes total.

New tooling landmine: rustfmt —skip-children false green

rustfmt --check --skip-children prints nothing on stable

--skip-children is UNRECOGNIZED on the pinned stable rustfmt (1.89.0 toolchain): rustfmt --edition 2021 --check --skip-children <file> errors out with Unrecognized option: 'skip-children' and emits NOTHING on stdout, which reads as “0 fmt diffs, all clean”. An earlier gate attempt was false-green this way before detection.

  • Check WITHOUT the flag: rustfmt --edition 2021 --check <file>.
  • rustfmt FOLLOWS mod declarations into child files, so attribute reported hunks by the filename in each Diff in <path>:<line>: header, not by the file you passed.
  • To separate pre-existing churn from branch-introduced churn, run the same check against the base revision’s copy of the file: git show <base>:<path> > /tmp/x.rs.
  • Recorded in mando AGENTS.md section 10 plus the landmines table, alongside the RTK and cargo-fmt landmines; Mando AGENTS Guide (mirror) resynced.

Rework executed (2026-07-13): feature/BE-3482-pymando-v2

The rework above was executed the same day, as a from-scratch rebuild rather than an in-place retitle. The old v1 branch and worktree were left untouched as fallback.

  • Branch: feature/BE-3482-pymando-v2 in worktree .worktrees/BE-3482-pymando-v2. 15 title-only, no-scope commits (author preserved, andras.lederer@alpiq.com) off origin/develop tip 51d2b516. develop advanced twice during the session (dff18d5f, then the BE-1595 Arrow Flight merge 51d2b516, which touches py-mando/src/lib.rs + py-mando/Cargo.toml); the rebuild targets the real tip. Zero cherry-pick conflicts. Diff vs develop: 10 files, +374/-55.
  • Commit selection: the 9 on-ticket commits replayed in original order, scopes stripped. 9b0c4e70 KEPT (on-ticket logger-init split with focused tests; touches only py-mando files; independent of all three dropped commits). DROPPED: 86d7ffd8 + 34e749d5 (the version fix rides MR !569) and d0986230 (fmt_util extraction, to be refiled as its own ticket).
  • Surprise: fe78dc10 needed NO level_to_status workaround. The predicted ~7-line temporary duplication never materialized: the helper (and its test) were already inline in py-mando/src/log_formatter.rs pre-extraction, since d0986230 is what moved them out. Dropping d0986230 therefore cost zero extra code.
  • All blockers and should-fixes executed: 7 orphaned ContextVars deleted (declarations, all set/reset lines in trace(), the dead metadata parameter, and the run_with_trace call site; zero readers remain); dead tracing-serde dep removed from py-mando/Cargo.toml (py-mando-simulation + workspace entry untouched, genuinely used there); the fingerprint outside a trace scope now emits the bare code instead of code|None (new test test_log_error_without_step_context_omits_path asserts no "None"); build_ddtags made public (tracing.py is a legitimate cross-module caller; no alias needed, no test referenced the private name); test hygiene done for real: pytest’s caplog replaces the hand-rolled _Capture, an error_log fixture collapses the three duplicated setups, a logger_state fixture genuinely restores lg.metadata / lg.identity / lg._initialized / root handlers / root level, and DD_ENV + DD_VERSION are monkeypatch-tracked. Tests now use the real tr.trace(...) API instead of poking context_var directly.
  • Deliberately NOT actioned (all flagged as judgement calls in the review): the _push_ddtags_to_rust ImportError swallow, set_ddtags discarding its Result (first-write-wins, has a test), and set_log_tags left without a doc comment per AGENTS.md 2.11 (no comments in new code). The dd_formatter.rs / api_docs.rs style nits are N/A: those commits were dropped.

New field: flow.step.system

flow.step.connection does not exist in the py-mando surface at all. It is produced in exactly one place, the step info_span! in mando-lib/src/workflow/mod.rs, built from structured data: StepMetadata { flow: DataFlow, system: &'static str }. The new field is therefore ONE line at the production site, flow.step.system = metadata.system, taken from the structured source with no string parsing of the connection value.

  • Verified against the generated flows: all 14 DataFlow::Wrapper (internal) steps declare system: "Mando", no exceptions; external steps declare the real counterparty (Volue EMS x19, Metis x14, Position Manager x8, …).
  • Semantics, all four required cases: Receive Volue EMS (connection Volue EMS => Mando) → Volue EMS; Receive Metis → Metis; Send Volue EMS (connection Mando => Volue EMS) → Volue EMS; Wrapper (connection Mando) → Mando.
  • Tests: new step_span_system_is_the_non_mando_counterparty, a #[test_case] matrix over exactly those four rows asserting both flow.step.connection and flow.step.system; the pre-existing step_span_carries_step_name_and_connection extended to emit the new field. All 4 cases pass.

Scope tension (flagged, unresolved)

This puts one mando-lib commit (52b4d270) on an otherwise py-mando-only branch. It is a single self-contained commit and trivially splittable onto its own branch if the MR should stay pure py-mando.

Gates (all green)

  • clippy (rtk proxy cargo clippy --release --all-features): exit 0, zero findings in branch-touched files (remaining warnings are pre-existing, in untouched crates).
  • cargo tests (rtk proxy cargo test --all-features --release -- --test-threads=1): 434 passed / 0 failed / 35 ignored; nonzero totals confirm the RTK false green did not occur; all new tests confirmed executing in the run.
  • rustfmt: zero branch-introduced hunks, measured in place at base vs branch (the 3+1 pre-existing hunks in py-mando/src/lib.rs / log_formatter.rs are byte-identical on origin/develop; left alone per “hand-fix only your own lines”).
  • pytest (poetry run pytest --nbval): 120 passed / 1 skipped (handover baseline was 114 / 1), including the new conformance tests and a runtime JSON conformance proof captured from the Rust stream (status, logger.file, logger.line, ddtags all present in the emitted line).
  • Cargo.lock carries one extra hunk (mandarrow-client 1.16.0 → 1.16.1) beside the tracing-serde removal because develop’s committed lock is stale; see the landmine below. Kept rather than hand-editing the lock to a state cargo immediately overwrites.

Two more tooling landmines (recorded in AGENTS.md section 10 + landmines table; mirror resynced)

rustfmt base-file checks in a scratch dir are FALSE GREEN, and rustfmt writes into child files

Copying a base-revision file to a scratch dir and running rustfmt --check there reports 0 hunks no matter what: rustfmt follows mod declarations, cannot resolve the children from the new location, and silently passes. Worse, running rustfmt (non-check) on a module-declaring file writes into its resolved child files: during this rework it swept develop’s pre-existing flight-import churn into the diff and touched 6 unrelated child files (algo.rs, algo_runner.rs, model/cache_mode.rs, polars.rs, test.rs, validation.rs), all reverted. The only sound method: check out the base revision of the file AT ITS REAL PATH and run rustfmt there.

develop's committed Cargo.lock can be stale

mandarrow-client is version.workspace = true (= 1.16.1) but develop’s committed lock says 1.16.0, so any plain cargo build on clean develop regenerates the lock. A Cargo.lock hunk in a branch diff may therefore be legitimate, not churn.

Environment notes

  • ~/.local/bin/python3.12 is broken (its venvs crash in ensurepip); use /opt/homebrew/bin/python3.12 instead.
  • A transient SIGBUS hit /Volumes/bandi during maturin develop --release (I/O fault on the external volume, not a code error); the retry succeeded.

State

Nothing pushed for v2, no MR. The old feature/BE-3482-pymando branch + .worktrees/BE-3482-pymando worktree are untouched as fallback; the main checkout is untouched (still on bugfix/BE-3333). feature/BE-3482-fingerprint-activity is now pushed and open as MR !569 (“feat: add activity component to error fingerprint”, 2 commits incl. the salvaged panic fix), awaiting review.

Final outcome (2026-07-22)

Both MRs merged: !569 (merge 53912a70, squash b3f39605, by 2026-07-15) and !570 = this rework (merge 14dfb3ad, squash fde2425e “feat: pymando logging conformance”). Wheel py-mando==1.16.1+dev.2691658359.14dfb3ad published to Nexus dev; the v1 fallback branch/worktree are archivable. Merge-train detail: Merge train completed (verified 2026-07-22). Consumer adoption: BE-3613 Algo Services py-mando Conformance.

Graph View

Backlinks