fix(coordinator): route target_reached through CLOSE so the close seq…

[WhatGhost]

Problem

When a run meets its --target-gain, the coordinator breaks out of the tick loop immediately, so the CLOSE phase never runs and the 7-step close sequencer is skipped. The session ends with only the CLI safety-net report:

{ "report_complete": false, "safety_net": true, "stop_reason": "target_reached" }

# Inference Optimizer — emergency final report
> **Auto-generated safety-net.** The CLOSE phase 7-step sequencer did not run to
> completion (process exited before phase transition, or `report` executor failed).

A successful run therefore loses the real report output, the post-optimization roofline (CLOSE step 0), kernel_optimization_summary.json, robustness_postmortem.md, and the CLOSE segment in session_breakdown.json.

Root cause

machine_state.py:228 registers target_reached as an "any phase -> CLOSE" terminal transition reason, but nothing ever produced that transition — compute_next_phase() never returns it. The only producer was the tick loop, which used it as a local variable and broke:

if objective.reached(self.shared_state):
    stop_reason = "target_reached"   # local variable only
    break                            # phase machine never runs again

_advance_phase_if_needed() lives in the tick body, so break removes any further chance to reach CLOSE — and shared_state.stop_reason is still empty at that point, since it is only written in the finally block.

This also made the two success terminals inconsistent: time_exhausted (also in _SUCCESS_STOP_REASONS) routes through a closing path, so a timeout closed cleanly while a met target did not.

Fix

Publish the stop reason and let the existing state machine drive the transition:

if objective.reached(self.shared_state):
    if not self.shared_state.stop_reason:
        self.shared_state.set_stop_reason("target_reached")
        self.shared_state.save(self.session_dir)
        continue
    stop_reason = self.shared_state.stop_reason
    break

_global_terminal() (machine_state.py:1645) already picks this up — its comment reads "Coordinator-set stop_reason takes precedence over phase exits". That path had simply never been fed a value. No new mechanism is introduced.

Bounded: after continue, the if self.shared_state.stop_reason and not in_closing check necessarily fires on the next tick, so the loop runs at most one extra tick and cannot spin regardless of whether CLOSE succeeded. The else branch preserves the previous behaviour when a stop reason is already published, leaving the timeout path untouched.

Verification

Unit test. test_target_reached_routes_through_close_phase builds a run that necessarily meets its objective and asserts the phase reaches CLOSE. Reverting only the implementation confirms it catches the bug:

without fix:  FAILED  assert 'FRAMEWORK_AGENT' == 'CLOSE'
with fix:     PASSED

Full suite. 9 failed, 14360 passed, 32 skipped — the same 9 pre-existing failures as main; passing count goes 14359 -> 14360 (the new test).

End-to-end. A real run (MI355X, vLLM 0.24.0, Qwen3-8B, TP=1, conc=64, ISL=OSL=1024, bf16, --target-gain 15) ending in target_reached:

close_sequence_done:       True
cumulative_gain_validated: 28.12%   (baseline 6736.07 tok/s/GPU)

PRELUDE   from=-         reason=prelude_done
EXPLORE   from=PRELUDE   reason=target_reached
CLOSE     from=EXPLORE   reason=(current)

reason=target_reached on the EXPLORE segment is the vocab entry that previously had no producer.

	before	after
final.md heading	— emergency final report	Inference Optimizer Report — <id>
final.json	2,869 B	59,898 B
report_complete / safety_net	false / true	absent
CLOSE phase segment	missing	present
kernel_optimization_summary.json	missing	written
robustness_postmortem.md	missing	written

[xiaofei-zheng]

Code Review

Summary

Solid fix. The root-cause analysis is accurate: target_reached was registered in machine_state as a terminal transition to CLOSE, but the coordinator only set a local stop_reason and breakd, so _global_terminal() never fired and the 7-step close sequencer was skipped.

Publishing stop_reason and continue lets the existing state machine drive any phase → CLOSE on the next tick. The close sequencer runs synchronously inside _on_enter_close() during phase advance, before the loop-end stop_reason check — ordering is correct. The else branch preserves prior behavior when stop_reason is already set (resume path).

E2E evidence in the PR description is convincing (close_sequence_done: True, artifact sizes, CLOSE segment in session_breakdown.json).

---

Findings

1. [Medium] Unit test uses the wrong state field

In test_target_reached_routes_through_close_phase:

c.shared_state.cumulative_gain_validated = 50.0

But TargetGainObjective.reached() reads state.cumulative_gain, not cumulative_gain_validated (see objective.py). The existing pattern in test_objective.py::test_run_stops_on_objective_reached correctly uses:

c.shared_state.cumulative_gain = 50.0

With cumulative_gain left at the default 0.0, objective.reached() stays false and the run exits via max_ticks=6 with reason "max_ticks", not "target_reached". Please switch to cumulative_gain.

2. [Low] Test assertion is weaker than the fix target

The test only asserts phase == "CLOSE". It does not assert close_sequence_done is True, which is the actual outcome that distinguishes a real close from the safety-net path. Recommend adding:

assert c.shared_state.close_sequence_done is True

3. [Low] Resume with pre-set stop_reason still skips CLOSE (pre-existing)

test_run_preserves_prior_stop_reason_when_loop_exits_without_new_reason shows that a session resumed with stop_reason="target_reached" exits on the first stop check without entering CLOSE. Not introduced by this PR, but worth a follow-up if resume-after-interrupt is a common path.

4. [Nit] One extra tick of reactor work after target is met

The continue design defers the CLOSE transition to the next tick, so reactors/dispatcher/framework pump still run once after the target is hit. Functionally fine; acceptable trade-off for reusing existing machinery.

5. [Nit] Coordinator.run() docstring could mention CLOSE routing

The docstring still lists target_reached alongside time_exhausted (via closing phase) without noting that target_reached now routes through the CLOSE phase sequencer (a different mechanism from closing_phase grace). Minor doc sync.

---

Verdict

Approve with minor test fix. Logic is correct and the change is minimal. Please fix cumulative_gain_validated → cumulative_gain in the new test (and consider asserting close_sequence_done) before merge.

[WhatGhost]

Code Review

Summary

Solid fix. The root-cause analysis is accurate: target_reached was registered in machine_state as a terminal transition to CLOSE, but the coordinator only set a local stop_reason and breakd, so _global_terminal() never fired and the 7-step close sequencer was skipped.

Publishing stop_reason and continue lets the existing state machine drive any phase → CLOSE on the next tick. The close sequencer runs synchronously inside _on_enter_close() during phase advance, before the loop-end stop_reason check — ordering is correct. The else branch preserves prior behavior when stop_reason is already set (resume path).

E2E evidence in the PR description is convincing (close_sequence_done: True, artifact sizes, CLOSE segment in session_breakdown.json).

Findings

1. [Medium] Unit test uses the wrong state field

In test_target_reached_routes_through_close_phase:

c.shared_state.cumulative_gain_validated = 50.0

But TargetGainObjective.reached() reads state.cumulative_gain, not cumulative_gain_validated (see objective.py). The existing pattern in test_objective.py::test_run_stops_on_objective_reached correctly uses:

c.shared_state.cumulative_gain = 50.0

With cumulative_gain left at the default 0.0, objective.reached() stays false and the run exits via max_ticks=6 with reason "max_ticks", not "target_reached". Please switch to cumulative_gain.

2. [Low] Test assertion is weaker than the fix target

The test only asserts phase == "CLOSE". It does not assert close_sequence_done is True, which is the actual outcome that distinguishes a real close from the safety-net path. Recommend adding:

assert c.shared_state.close_sequence_done is True

3. [Low] Resume with pre-set stop_reason still skips CLOSE (pre-existing)

test_run_preserves_prior_stop_reason_when_loop_exits_without_new_reason shows that a session resumed with stop_reason="target_reached" exits on the first stop check without entering CLOSE. Not introduced by this PR, but worth a follow-up if resume-after-interrupt is a common path.

4. [Nit] One extra tick of reactor work after target is met

The continue design defers the CLOSE transition to the next tick, so reactors/dispatcher/framework pump still run once after the target is hit. Functionally fine; acceptable trade-off for reusing existing machinery.

5. [Nit] Coordinator.run() docstring could mention CLOSE routing

The docstring still lists target_reached alongside time_exhausted (via closing phase) without noting that target_reached now routes through the CLOSE phase sequencer (a different mechanism from closing_phase grace). Minor doc sync.

Verdict

Approve with minor test fix. Logic is correct and the change is minimal. Please fix cumulative_gain_validated → cumulative_gain in the new test (and consider asserting close_sequence_done) before merge.

@xiaofei-zheng Thanks for you review~. I'll read and fix them.

[WhatGhost]

@xiaofei-zheng Thanks for the review. I've read and fixed them. Please review again.

For 1 — This may be based on an earlier revision. On current main, TargetGainObjective._current() returns cumulative_gain_validated (objective.py:141-143), and test_objective.py:239 sets that same field. I tried the suggestion locally: switching to cumulative_gain makes the run exit with assert 'max_ticks' == 'target_reached'. Keeping cumulative_gain_validated — happy to look again if you're seeing something different.

For 2 — Agreed, real gap. Added assert c.shared_state.close_sequence_done is True, which is the flag the CLI finally block uses to decide on the safety-net write.

For 3 — Reproducible, and a consequence of the else branch kept for backward compatibility. Pre-existing, so I'd rather not fold it into this PR. Happy to open a separate issue.

For 5 — Fixed: target_reached (via the CLOSE phase sequencer), to distinguish it from time_exhausted, which goes through _enter_closing_phase().