Greptile Summary

This PR fixes a regression where a StepMutator's mutate() was called multiple times — once by an early _process_late_attached_decorator pass (e.g. conda/pypi flow_init) and again by the subsequent _init_step_decorators call — causing allow_multiple decorators such as @card to be duplicated on the step.

• Adds _mutate_called / _mutate_inserted tracking to UserStepDecoratorBase, extracts _run_step_mutator and _remove_step_mutator_outputs helpers, and guards _init_step_decorators to skip mutators that already ran in a late-attach pass.
• _process_late_attached_decorator now removes the mutator's previous output before re-running it (when already_ran=True), then tracks the new outputs for re-initialization — correctly supporting both the init-before-late (Argo/Kubernetes) and late-before-init (conda/pypi) orderings.
• Four new unit tests cover the regression scenario, the distinct-late-attach re-run guarantee, and replacement-output initialization.

Confidence Score: 5/5

The change is safe to merge; the fix is minimal and well-tested across the two known execution orderings.

The dual-ordering logic (_mutate_called guard in _init_step_decorators + remove-then-rerun in _process_late_attached_decorator) correctly handles both the conda/pypi and Kubernetes/Argo lifecycles. New tests directly reproduce the regression and verify the re-initialization of replacement outputs. No pre-existing invariants are broken.

No files require special attention; the two duplicate test cases in test/unit/test_late_attached_mutator.py are a minor redundancy but do not affect correctness.

Important Files Changed

_{Reviews (7): Last reviewed commit: "add more unit tests for coverage" | Re-trigger Greptile}

Canon PR Review — Engineering Standards Scorecard

PR: fix: regression with StepMutator running multiple times in some cases
Reviewed against: Top 20 Tier 2 practices (Code Complete, Clean Code, APoSD, Refactoring, Release It!, DDIA, The Pragmatic Programmer, Working Effectively with Legacy Code, Design Patterns GoF)

Scores

Decision: APPROVE

Violations Found

Module Design

CC-028 — Interface Comments (what + why) (Source: A Philosophy of Software Design)

• Location: metaflow/user_decorators/user_step_decorator.py — class body of StepMutator
• Issue: The two new class-level flags _mutate_called and _late_mutate_called lack a comment explaining their semantics as per-instance idempotency guards and, critically, the assumption that each decorator instance is used in exactly one flow invocation. Without this, a reader could reasonably wonder: (a) why these are class vars and not instance vars initialized in __init__; (b) whether reuse of the same decorator object across multiple flow invocations in the same process would silently skip mutation (it would).
• Concern: Python will correctly shadow the class var with an instance attribute on first deco._mutate_called = True, so the guard logic itself is correct. The issue is that the flags are never reset. In typical CLI usage this is fine (each python flow.py run is a fresh process). But in notebook or programmatic multi-run patterns, the same @MyMutator() instance persists across calls and the second run's mutator would be silently skipped.
• Suggested improvement:

  # Per-instance flags tracking whether mutate() has been called via
  # _init_step_decorators (_mutate_called) or _process_late_attached_decorator
  # (_late_mutate_called).  These intentionally live at class level so that
  # reading the attribute before the first assignment works without __init__
  # changes, relying on Python's instance-attribute shadowing on first write.
  # NOTE: these are never reset — assumes each decorator instance is used in
  # a single flow invocation (true for CLI; not true for multi-run notebooks).
  _mutate_called = False
  _late_mutate_called = False

Maintainability

CC-093 — Fix Broken Windows (Source: The Pragmatic Programmer)

• Location: PR description
• Issue: The PR template's Summary, Reproduction, Root Cause, Why This Fix Is Correct, and Failure Modes Considered sections are all empty. For a subtle call-stack-order bug in a shared primitive (StepMutator), the root cause and the invariant being restored are non-obvious and would be valuable to preserve alongside the fix.
• Suggested improvement: Fill in at minimum Root Cause (the ordering inversion between _process_late_attached_decorator and _init_step_decorators in conda/pypi's flow_init) and Why This Fix Is Correct (the two-flag asymmetry: _init_step_decorators checks both flags; _process_late_attached_decorator checks only _late_mutate_called so it can still re-run when kubernetes is genuinely late-attached).

What's Working Well

1. Idempotency fix is precise and minimal — The two-flag design elegantly handles the asymmetric call orders. _init_step_decorators stops on either flag; _process_late_attached_decorator only stops on its own flag so it can still re-run when kubernetes is genuinely late-attached and always cleans up previously-inserted decorators before re-running. This is exactly right for the problem.

2. Regression test precisely demonstrates the bug — test_allow_multiple_decorator_not_duplicated_on_mutator_rerun sets up the production call order (late-attach before init, as with conda/pypi), exercises allow_multiple decorators (@card), and asserts the exact invariant. It would fail on master without the fix and passes after. The docstring explaining the production scenario is a genuine asset.

3. Guard clauses via continue — Both guard blocks use early continue at the top of the loop body, keeping the happy path at leftmost indentation and making the intent clear (CC-011).

To Unblock

Nothing is blocking merge. Two follow-ups worth tracking (not required for this PR):

1. Consider adding a comment (or docstring) to the two new class-level flags explaining the instance-shadowing assumption and the no-reset caveat for programmatic multi-run usage.
2. Fill in the PR description's root cause and fix rationale — valuable for git log archaeology when this area next changes.

Scored against: CC-003, CC-005, CC-006, CC-011, CC-021 (Code Quality) | CC-027, CC-015, CC-028, CC-017 (Module Design) | CC-014, CC-048, CC-056, CC-058, CC-060 (Reliability) | CC-019, CC-031, CC-093, CC-107 (Maintainability)

Discussed async - details here.

Follow-up: Updating decision to REQUEST_CHANGES

<@talsperre> shared a detailed counter-review with 4 findings. After cross-checking against the head commit (ff68ea9), here's where I land:

Finding 1 (late-before-init still duplicates) — Disagree on current code

The gist says _init_step_decorators re-runs mutate() even when _late_mutate_called=True. In the head commit, line ~944 reads:

if deco._mutate_called or deco._late_mutate_called:
    continue

So if the late pass ran first and set _late_mutate_called=True, _init_step_decorators skips the mutator. This ordering (conda/pypi) should produce exactly one @card. The gist may have reviewed an earlier commit before "combine guards and cleanup" landed.

However, I'd recommend a targeted test to confirm — _call_init_step_decorators after _call_process_late_attached with a @card mutator and asserting count == 1.

Finding 2 (name-based cleanup deletes wrong outputs for two instances of same class) — Confirmed

decorator_name is class-level ({module}.{ClassName}), not per-instance. The cleanup in _process_late_attached_decorator:

d.inserted_by[0] == mutator_name

...uses deco.decorator_name which is shared by all instances of the same class. Two @AddCardMutator(id="card1") and @AddCardMutator(id="card2") on the same step would have their outputs cross-deleted. Uncommon but real.

Finding 3 (late-pass re-run leaves replacement decorator uninitialized) — Confirmed, more critical

Scenario B (normal init ran first, then kubernetes late-attaches):

1. _init_step_decorators runs → mutator adds @card → @card gets external_init + step_init at lines 984–1011 ✓
2. _process_late_attached_decorator runs → cleanup removes the initialized @card → mutator re-adds a fresh @card → _late_mutate_called = True
3. Late-pass init loop (lines 1108–1126): if deco.name in deco_names → only initializes "kubernetes", not "card"

The replacement @card is never external_init'd or step_init'd. This is a silent failure — the card object exists in step.decorators but its _ran_init is False and its internal state is uninitialized. Depending on what CardDecorator.step_init does, this could silently produce no card or error at task time.

Finding 4 (test misses Findings 2 and 3) — Confirmed

The existing test only asserts card_count == 1 for inserted_by objects. It passes even when the surviving card is the uninitialized replacement from Finding 3.

Updated decision: REQUEST_CHANGES

Finding 3 is the blocking issue. The proposed fix in the gist (id-based tracking + explicit re-init of replacement outputs) addresses it correctly. At minimum, the late-pass re-run path needs to call external_init/step_init on any decorator the mutator freshly inserts (the ones whose initialized predecessors were just removed).

Scored against: CC-014 (silent failure mode on uninitialized decorator — Reliability), CC-107 (regression test doesn't catch the fixed behavior — Maintainability)