fix(autopilot): offload completion result reads

[leonlaiyc]

Problem / Motivation

_stage_loop in src/kiro_crew/dashboard/chat_orchestrator.py is an async def
that drives a plan stage by stage. When the for loop completes without a break
— every stage ran — it falls into the else branch and builds the terminal
"✅ All N stages complete." summary.

That summary is assembled by re-reading the captured stage results off disk:

for s_idx in range(total):
    result_path = tracker._stage_results.get(s_num)
    if result_path:
        try:
            text = safe_read_file(result_path).strip()

safe_read_file is synchronous. It calls os.path.realpath, checks
is_sensitive_path, then os.open(..., O_NOFOLLOW) and reads the file to
completion. Nothing about that call yields to the event loop, and the branch runs
one such call per completed stage — so a plan's entire final read set is issued
back to back, on the loop, at the moment the plan ends.

This is a different lifecycle boundary from the per-stage previous-result reads
in #3772. Those happen before each next stage and feed that stage's prompt;
these happen after the whole loop and feed the final user-visible message. The
caller branch, the timing, the consumer, and the failure semantics are all
separate, so this is verified on its own.

Why it matters

Scheduling only. The gateway runs a single asyncio event loop, and for the
duration of these filesystem operations that loop cannot advance anything else —
no WebSocket broadcast, no other slot's turn, no HTTP handler. The number of
reads scales with the plan's stage count, and they all land in the same
uninterrupted window.

No latency figure is claimed here; none was measured.

What changed

The read set is moved off the loop without moving any state with it.

• Loop: snapshot (stage number, path) pairs out of
  tracker._stage_results into an immutable tuple. The tracker's result map is
  live orchestration state the loop mutates via record_stage_result, so it is
  read on the loop thread only.
• Worker: a new module-level _completion_excerpts(...) takes that tuple,
  performs the safe_read_file calls, derives each stage's excerpt, and returns
  a plain dict[int, str]. It touches no slot, no tracker, and no live flag.
• Loop: consumes the returned mapping to build summary_lines, then runs
  redaction, slot.append, broadcast_ws, and the SEL auto_run_completed
  record exactly as before.

A plan that captured no result paths skips the asyncio.to_thread hop entirely
rather than paying for an empty round trip.

Semantics are preserved deliberately: excerpt selection (first non-empty line
that does not start with ───), the 120-character cap, stage ordering, the
per-stage containment of a read error, and the — done fallback for a missing or
unreadable result are all unchanged. A stage whose read fails is simply absent
from the returned mapping, which reproduces the old excerpt = "" fallback.

No new executor, abstraction, caching, retry, or error-handling policy is
introduced, and no truncation limit or result content changes.

Testing

New test/test_completion_result_read_off_loop.py drives the real _stage_loop
to plan completion and wraps the production safe_read_file binding in
chat_orchestrator with a recorder that delegates to the real implementation —
so the recorded thread is the thread that genuinely opened and read the file, not
merely one that reached a call site. The observation is scoped to the stage-result
paths, and a non-empty guard makes a vacuous pass impossible.

Deterministic fail-before / pass-after on this branch:

	before	after
test_completion_result_read_runs_off_the_loop_thread	FAIL — assert 62540 not in [62540, 62540, 62540]	PASS
test_completion_reads_every_captured_stage_result	FAIL — same loop-thread id, 3 reads	PASS

The remaining eight cases are preservation coverage and pass on both trees, which
is what pins the behaviour this change must not alter: ordering and titles,
separator-line skipping, the 120-char cap, blank-result fallback, a deleted
result file degrading to — done while its sibling still resolves, credential
redaction of the excerpt, the no-worker-hop path when nothing was captured, and
terminal _auto_run state.

pytest test/test_completion_result_read_off_loop.py     10 passed
pytest test/test_dashboard_chat.py \
       test/test_display_time_redaction.py             587 passed, 1 skipped

Also green: flake8, isort --check-only, mypy --platform linux on the
changed module ("Success: no issues found"), scripts/docs_lint.py,
BRAND_BASE_REF=origin/main scripts/check_brand_name.py, git diff --check.

Screenshots / video

Backend scheduling only — the completion message's text is byte-identical.

Related Issues

None. Self-discovered while validating the sibling event-loop I/O findings in
#3772.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 64c34d36e4de78c4e0e48b3ba3f9f279d70419de via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 64c34d3

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Advisory design-level review of 64c34d36e4de78c4e0e48b3ba3f9f279d70419de via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: PASS

Sound, minimal offload following the established #3772 pattern — snapshot on loop, immutable data to worker, semantics pinned by tests.

Watch

• The asyncio.to_thread hop opens a new await window between "all stages done" and the summary append/broadcast; a slot stopped or deleted during that window (mid-plan deletion is a real event — the finally handoff already guards state._slots.get(slot.key) is slot) will still get its summary appended and broadcast to a deregistered key. Harmless today, but a one-line re-check of _stopping/registration after the await would close it.

Suggestions

• _capture_stage_result has the full result text in memory at write time; recording the first-line excerpt in the tracker alongside the path would eliminate the completion re-read entirely (same in-memory durability as _stage_results) — a follow-up that removes the I/O rather than relocating it.

[DESIGN-REVIEWED] 64c34d3

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 64c34d36e4de78c4e0e48b3ba3f9f279d70419de via the fork AI-review pipeline; updated in place on each push.

Review details

The diff is a faithful, behavior-preserving refactor. I verified the new _completion_excerpts produces identical output to the old inline loop: same path snapshotting, same first-non-empty-non-─── line selection, same [:120] cap, same (OSError, PermissionError) handling mapping to a missing key → "" → — done fallback, and redaction still runs on the loop over done_msg. No mutable state crosses the thread boundary, and the empty-capture shortcut is equivalent to the old result_path is None path. The discovery pass found no candidates and my independent falsification confirms nothing survives at 80+.

No findings.

[OPUS-REVIEWED] 64c34d3

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Advisory premise-level review of 64c34d36e4de78c4e0e48b3ba3f9f279d70419de via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

I've read the contract, intent, patch, and the base chat_orchestrator.py, checked safe_read_file in hooks.py, and confirmed the sibling change #3772 is merged on origin/main with the identical snapshot-then-to_thread shape. I counted the remaining synchronous filesystem calls in _stage_loop's own path. Final review follows.

First-Principles-Verdict: CONCERNS

The offload is the recorded #3772 decision applied at its declared sibling site — but the same function still does heavier sync disk I/O per stage, unmentioned.

What this change ships

Intent: keep the gateway event loop responsive while the plan-completion summary re-reads stage results off disk — a FIX (scheduling defect).

1. Completion-summary file reads now run on a worker thread — justified (asyncio blocking rule; decision recorded at fix(autopilot): offload previous-result reads #3772)
2. Empty-capture plans skip the worker hop — declared, rides along, inherited by symmetry with fix(autopilot): offload previous-result reads #3772
3. New module-level _completion_excerpts — one consumer (the completion branch), required by the to_thread handoff
4. Summary text, ordering, caps, fallbacks byte-identical — declared preservation, pinned by 8 tests

Watch

• Point patch with counted unfixed siblings: grepping write_text|mkdir|\.load\( in the _stage_loop call path finds 2 sync sites still on the loop — _capture_stage_result's mkdir + write_text of the full stage transcript (chat_orchestrator.py:135-137, called per stage at :517) and KiroCrewConfig.load() (:154). The write is per-stage and as large as the reads this PR moves. The description scopes itself against fix(autopilot): offload previous-result reads #3772 but never says the write side is left on the loop.

Subtractions

• Drop the if _captured: skip guard and test_no_worker_hop_when_no_stage_results_were_captured. In fix(autopilot): offload previous-result reads #3772 the empty case is every plan's first stage; here it means every stage's disk capture failed — a rare error path. The guard is symmetry with fix(autopilot): offload previous-result reads #3772, not a cost at this site, and its test monkeypatches asyncio.to_thread globally to pin a micro-optimization.

[FIRST-PRINCIPLES-REVIEWED] 64c34d3

[iamwhatever]

Tier 1 auto-approve: perf (2 files). Criteria: no conflict, no requested changes, no security surface, AI reviewers green. Category: offloads completion-result file reads off the autopilot event loop — pure performance fix with matching test, no behavior change.