Skip to content

merge_interventions lets an empty-string stop_reason shadow a later real one #5

Description

@zhanghanduo

agent_core/loop_types.py merges stop reasons with a None check:

if stop_reason is None and iv.stop_reason is not None:
    stop_reason = iv.stop_reason

Intervention.stop_reason is str | None, so an observer returning Intervention(stop_reason="") — e.g. a reason built from a config value that defaulted empty — claims the slot.

Consequence: a later observer's real stop_reason="budget_exhausted" is discarded, and a loop testing truthiness (if merged.stop_reason:) then sees no stop at all — the run continues past both stop requests. A budget or deadline observer silently stops working.

Fix direction: treat the empty string as absent — normalise "" to None at merge, or gate on truthiness. The docstring rule ("first non-None wins") should be restated as "first non-empty wins". Worth a test in tests/test_loop_types.py pinning the shadowing case.

Provenance: pre-existing in ApodexHarness/miroharness/core/loop_types.py, ported verbatim by #1 — not introduced by the refactor. ApodexHarness carries the same code and needs the same change until it consumes AgentCore.

Activity

  1. zhanghanduo commented on Sep 3, 2026

    @zhanghanduo
    CollaboratorAuthor

    Fixed on main by 63dba6f (not auto-linked — that commit's Refs trailer sits after a literal \n\n).

    merge_interventions now gates on truthiness, so "" is treated as absent (agent_core/loop_types.py:606):

    if stop_reason is None and iv.stop_reason:
        stop_reason = iv.stop_reason

    The docstring rule was restated as the issue asked — "take the first non-empty stop". The shadowing case is pinned by tests/test_loop_types.py::test_empty_stop_reason_does_not_shadow_later_reason (:110), alongside test_stop_reason_first_non_none_wins (:99) so neither direction can regress silently.

    Downstream: miroharness/core/loop_types.py:777 still has the is not None form. Tracking that backport separately.

  2. zhanghanduo commented on Sep 3, 2026

    @zhanghanduo
    CollaboratorAuthor

    Correction to the "Downstream" note above.

    That note is accurate for MiroHarness main, which still carries the standalone 790-line copy — but it is already stale for the integration branch. On refactor/agent-core, miroharness/core/loop_types.py is a 75-line compatibility facade that re-exports agent_core.loop_types (done by 1bedcc52 refactor: consume shared runtime contracts), and that branch pins:

    apodex-agent-core @ git+ssh://git@github.com/ApodexAI/AgentCore.git@a9b5272d
    

    git merge-base --is-ancestor 63dba6f a9b5272 confirms the pin already contains this fix.

    So no backport is needed. The downstream copy is not being patched in parallel; it is being deleted by the extraction. MiroHarness main keeps the old code only until refactor/agent-core lands, and this fix arrives there through the pin rather than through a second edit.

    The only product-specific symbol that stays behind in the facade is wall_deadline_remaining_s, which binds miroharness.core.execution_context and is registered as an allowed delta in scripts/contract/manifest.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions