Fix graph progress state after interruption

[Sanjays2402]

Closes #415.

Full and incremental graph builds now emit a terminal graph progress event from a finally block, so interrupted subprocesses stop the renderer instead of leaving a running task. Regression tests cover failed events on interruption and done events on success for both runners; all 11 pipeline tests and changed-file Ruff checks pass.

[HumanBean17]

Code Review — Fix graph progress state after interruption

The interrupt-path fix is correct and well-tested. Three findings, most severe first.

1. Parent terminal graph event duplicates the child's own done on success — pipeline.py:479 / :558

The child build_ast_graph.py::_graph_pass_progress emits JCIRAG_PROGRESS kind=graph pass=6/6 status=done in its finally on every successful pass 6. The default path passes --verbose, so this line is parsed by ProgressRelay and delivered to on_progress. The renderer's apply() keys only on ev.kind and treats any status="done" as terminal (graph ✓, stop_task) — it does not check pass_. So on success the renderer already receives a terminal graph event from the child; the parent's finally then fires a second one.

• TTY: the second apply is an idempotent no-op (task already stopped). Fine.
• Non-TTY (CI / piped stderr): _fallback_apply bypasses the throttle for terminal events → a second graph done concise line is printed.
• Programmatic on_progress consumers (MCP / events.append-style — exactly what the PR's test simulates): receive two terminal events for one build, breaking the "one terminal event per kind" invariant.

The added test misses this because it mocks _popen_capturing_stderr to return ("", "", 0), so no child events flow.

This was copied from the cocoindex pattern, whose comment explicitly justifies the parent-side event because "the flow cannot emit the terminal vectors event." The graph child can and does.

Suggested fix: emit the parent's terminal event only on failure/interruption (if code != 0), or dedup terminal events per kind in the renderer.

A related edge: if pass 6 raises internally, _graph_pass_progress still emits pass=6/6 status=done unconditionally in its finally, so the renderer marks graph ✓ — then the child exits non-zero and the parent's finally emits status=failed → graph ✗ on top. Non-TTY prints graph done then graph failed.

2. Terminal-event emission is at the wrong altitude — three identical copies now exist — pipeline.py:470 / :549

All three callers of _popen_capturing_stderr now carry a byte-for-byte identical try/finally terminal-event block:

• _run_cocoindex_update_impl (:371, pre-existing)
• run_build_ast_graph (:470, new)
• run_incremental_graph (:549, new)

The shared helper already has on_progress in scope, sees the exit code at proc.wait(), and has an abort path that catches BaseException and re-raises — the natural injection point. Adding a kind: str parameter and emitting the terminal event inside _popen_capturing_stderr would:

• delete ~40 lines of triplication, and
• make fix(progress): graph/increment task stays "running" on Ctrl+C abort #415 structurally impossible to reintroduce for the next subprocess runner (today the helper's signature gives no hint that callers must wrap it).

The three copies already nearly drift: the cocoindex copy pre-initializes out_s, err_s = "", ""; the two new graph copies don't.

3. Stray blank line after inline import — pipeline.py:485 / :564

The diff adds a blank line after from java_codebase_rag.cli_format import ... inside the if not verbose: block in both functions. Unrelated formatting noise — drop it.

---

Minor / not blocking

• out_s/err_s are unbound if Popen itself raises, but the exception propagates past the return, so no NameError is reachable today. Pre-initializing (like the cocoindex twin) would be more robust if a spawn-failure path ever returns a CompletedProcess.
• The on_progress(...) inside finally could mask an in-flight KeyboardInterrupt if the renderer callback raises, but apply() is hardened to no-op after stop() and this matches the accepted cocoindex pattern. Low probability.

TL;DR

Interrupt fix is good, but the success path now double-fires a terminal graph event for non-TTY / programmatic consumers (the child's pass=6/6 status=done already terminals the kind; the renderer doesn't distinguish per-pass done from overall done). Gate the parent's emission on code != 0, or dedup in the renderer. Factor the triplicated try/finally down into _popen_capturing_stderr. Drop the two blank-line hunks.

🤖 Generated with Claude Code

[Sanjays2402]

Thanks for the detailed writeup — the duplicate-terminal-event analysis was spot on.

Finding 1 (the blocker): fixed. Gated the parent's terminal-event finally on on_progress is not None and code != 0 (status hardcoded to "failed"), so on success the child's own pass=6/6 status=done is the single terminal graph event the relay delivers, and the parent only emits on the failure/interrupt path (including spawn-failure where code stays -1). That restores the one-terminal-event-per-kind invariant.

The test miss: you nailed why it was invisible — the old success test mocked _popen_capturing_stderr to return ('','',0) so no child events flowed. Rewrote it so the fake capture emits the child's kind=graph pass=6/6 status=done, then asserts exactly one terminal graph event. Confirmed it fails (2 == 1) without the fix and passes with it.

Finding 3 (stray blank line): removed.

Finding 2 (hoist the triplicated try/finally into _popen_capturing_stderr): I held off on this one deliberately. It also touches the cocoindex caller, which legitimately needs a parent-side terminal event because that flow can't self-emit (and it fires even on spawn failure). Folding all three into the helper with a kind param risks regressing that path, so I'd rather do it as a focused follow-up than smuggle a structural refactor into this fix. Happy to pick it up separately if you'd like. 17 tests green.

[HumanBean17]

Verification — all three findings addressed ✅

Checked out f83b531 into a clean worktree, editable-installed, and verified each claim directly (not just from the diff).

Finding 1 (duplicate terminal event — the blocker) — FIXED & proven

The if on_progress is not None and code != 0 gate (status hardcoded "failed") is correct across all paths:

• Success (code == 0): parent no longer emits → the child's pass=6/6 status=done is the single terminal graph event. ✅
• Interrupt / spawn-failure (code stays -1): parent emits failed → original fix(progress): graph/increment task stays "running" on Ctrl+C abort #415 goal preserved. ✅

Regression test is real, not a tautology. I reverted the gate in run_build_ast_graph back to the old "always emit, done if code == 0" form and re-ran the success test — it fails with exactly the duplicate from the review:

AssertionError: assert 2 == 1
  [ProgressEvent(kind='graph', pass_='6/6', ..., status='done'),    # child
   ProgressEvent(kind='graph', pass_=None, ..., status='done')]     # parent (the bug)

Restored the fix → 4/4 graph-progress tests pass. The rewritten fake_capture (which emits the child's pass=6/6 done before returning ("", "", 0)) is exactly the right shape — it exercises the path the old mock hid.

Finding 3 (stray blank line) — FIXED

No blank line between the inline import and marker = ... in either runner. ✅

Finding 2 (hoist into _popen_capturing_stderr) — deferral accepted

Your reasoning is sound: the cocoindex caller legitimately needs a parent-side terminal event on all paths (including success) because that flow can't self-emit, whereas graph can. A single helper with one emission rule would either reintroduce the graph duplicate or need per-caller branching anyway — so it belongs in a focused follow-up, not smuggled into this fix. 👍

One narrow residual (non-blocking, child-side)

The done→failed flicker I mentioned as "a related edge" is not fixed by the code != 0 gate — in fact that gate is what produces it: if the child ever emits pass=6/6 status=done and then exits non-zero (pass-6 body raised but its finally still emitted done, or a post-pass-6 step failed), the renderer sees graph ✓ then graph ✗. That's a child-side bug (_graph_pass_progress emitting done unconditionally in its finally, even on exception), and the parent's final failed state is the correct one — so I wouldn't block on it here. Worth a separate look at _graph_pass_progress gating its done emission on whether the pass body raised.

Test suite

• tests/package/test_pipeline.py: 11 passed.
• ruff check on both changed files: clean.

Ship it. 🚢

🤖 Generated with Claude Code

[HumanBean17]

Approved. Thanks to contributing. Feel free to commit more or open issues with ideas or bugs :)