fix(autopilot): offload stage-result writes

[leonlaiyc]

Problem / Motivation

_stage_loop is async and drives the whole autopilot run, but stage-result persistence is synchronous. At every stage boundary _capture_stage_result calls session_dir.mkdir(parents=True, exist_ok=True) and path.write_text(...) directly on the gateway event loop.

Why it matters

The gateway runs a single asyncio loop, so those syscalls block every other task on it for as long as they take — chat streaming, WebSocket frames, cron dispatch. It happens once per stage, so a run with several stages pays it repeatedly, and mkdir on a cold or networked session directory is the slower of the two.

Scope note, stated deliberately: this is a scheduling defect. I have not measured the stall duration, so the claim here is only that unrelated loop work is blocked for the duration of the write, not that the duration is large.

What changed (motivation → approach → change)

Motivation — no part of stage-result persistence belongs on the loop.

Approach — draw the boundary at the filesystem, not at "the slow-looking calls". Everything that touches disk goes to one worker; everything that touches live slot state stays on the loop.

Change — the filesystem half moves into _write_stage_result, handed to asyncio.to_thread:

• stays on the loop thread — walking slot.messages back to the stage separator, collecting assistant content, redact_exfiltration_urls + redact_credentials, building result_text
• crosses into the worker — slot.key, stage_num, the finished immutable result_text, and an abandoned event; nothing else
• runs in the worker — mkdir, write_text, and the publishing os.replace

slot.messages is live mutable state; passing the slot itself would have turned a concurrent append into a cross-thread read for no benefit, so the worker never receives it.

Unchanged: the sessions/<slot_key>/stage_<n>_result.md path format, message ordering, redaction (still before any byte reaches disk), and failure semantics — to_thread re-raises, so the caller's except OSError still logs and continues. No retries, no dedicated executor, no filesystem abstraction.

The publication moved too, and why the earlier reasoning did not hold

An earlier revision of this PR kept os.replace on the loop on purpose, arguing that (a) a rename is metadata-only and therefore cheap, and (b) publishing strictly after an uncancelled await made the orphan-writer class structurally unreachable.

(a) is false in the case this offload exists for. The session directory can be network-backed, and there a rename is a server round trip like any other call — the same stall the rest of the change removes, and the same no-blocking-call-on-event-loop anchor covers it. Keeping it was importing the defect back at a smaller size.

(b) is real and is preserved — by a flag instead of by placement. asyncio.to_thread cannot interrupt a running worker, so the worker re-reads abandoned immediately before it publishes, and the caller sets that event the moment its await is cancelled (stop button, slot close, slot-key reuse). A capture abandoned during the payload write unlinks its temp file and leaves stage_N_result.md untouched. That is the whole of the window on the slow filesystem this change targets, and it is what test_cancelled_capture_never_publishes_the_stage_file pins — that test is unchanged and still passes.

What this does not claim: the check and the rename are two operations, so a cancellation landing between them still publishes. That residual window is one already-resolved rename rather than the entire payload write. It is stated rather than claimed away, and it cannot be closed by moving the rename back to the loop — while a loop-thread rename runs, the loop is not free to observe the cancellation either.

Two production functions, one test file.

Tests

test/test_stage_result_off_loop.py, eight tests:

Test	Pins
..._write_runs_off_the_loop_thread	the payload write lands on a non-loop thread
..._mkdir_runs_off_the_loop_thread	so does the directory creation
test_stage_result_publication_runs_off_the_loop_thread	new — so does os.replace, pinned separately because an implementation can satisfy the write assertion while still publishing on the loop, which is exactly the state this was added for
test_no_filesystem_call_reaches_the_loop_thread	new — the boundary itself rather than its current members: no mkdir, write_text or os.replace for this stage runs on the loop thread, so a future fourth syscall cannot be added on the loop without failing something
test_slot_messages_are_read_on_the_loop_thread	the offload stops at the filesystem — extraction stays on the loop
test_capture_preserves_path_ordering_and_redaction	path, oldest-first ordering, credential redaction
test_stop_landing_during_the_offloaded_write_does_not_advance	a stop landing during the offloaded write halts the run before the next stage
test_cancelled_capture_never_publishes_the_stage_file	a cancelled capture leaves the canonical stage file untouched — unchanged by this revision, and the reason the flag exists

Thread assertions are scoped to this stage's own directory and file, so unrelated filesystem traffic from fixtures cannot decide them, and each carries a non-empty guard so it cannot pass vacuously.

Fail-before / pass-after for the two new tests, with chat_orchestrator.py reverted to the previous head 16e9d15a6 and the tests left in place:

FAILED test_stage_result_publication_runs_off_the_loop_thread
  - AssertionError: the stage result was published on the event-loop thread; the
    rename is filesystem I/O and belongs in the same worker as the payload write
FAILED test_no_filesystem_call_reaches_the_loop_thread
  - AssertionError: stage-result capture performed filesystem work on the
    event-loop thread: os.replace

2 failed, 6 passed

The other six pass on both trees by design — including the cancellation test, which is the point: the orphan-writer guarantee is not weakened to buy the off-loop rename.

pytest test/test_stage_result_off_loop.py -q                                   8 passed
pytest test/test_stage_result_off_loop.py \
       test/test_completion_result_read_off_loop.py \
       test/test_display_time_redaction.py -q                                 42 passed

Gates: flake8 · isort --check-only clean on both changed files. test_stage_result_off_loop.py is not in .github/black-baseline.txt, so it is held to black and is formatted; chat_orchestrator.py is baselined and black --diff reports no hunk overlapping any changed region, so it is left unformatted rather than graduated off the baseline.

Manual verification

N/A — unit coverage sufficient: every claim here is a thread-identity or file-content property, and both are asserted directly against the real _capture_stage_result at the syscall boundary. Reproducing the stall by hand means running autopilot against a network-mounted session directory, which is the condition the tests encode rather than observe.

Related Issues

Refs #1783 — only the P2 bullet "_capture_stage_result does blocking disk I/O on the event loop". #1783 is an umbrella tracker whose body states each remaining finding is independently shippable; every other bullet is untouched and it stays open.

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — internal scheduling change, no user-facing behaviour documented
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: The branch conflicts with main (5 days of drift) and its last CI run failed on test/test_slack_gateway.py:4670, a service-init timing assertion (loop blocked for 1.87s, ceiling 0.40s) in a file this PR does not touch — an unrelated flake/base-drift, not a defect in this diff. Plan: rebase onto current main, resolve the conflict guided by the PR intent (async offload of stage-result writes, design unchanged), and re-run CI so the gated review bots (currently skipping) can run.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

Drive-to-green push 1 (head 781f24cd): rebased onto latest main (69b60c3b) to resolve the merge conflict.

Conflict + resolution — src/kiro_crew/dashboard/chat_orchestrator.py: since this PR was opened, main landed the read-side offload (_previous_result_paths + _read_previous_results, offloading previous-stage-result reads via asyncio.to_thread). This PR contributes the complementary write-side offload (_write_stage_result + async _capture_stage_result). The two features are disjoint; resolution keeps both: main's read-offload block and this PR's write-offload block now coexist, and the _stage_loop call site awaits _capture_stage_result as this PR intended. No design change, no scope added.

Authorship: original commit by Leon preserved as git author; Co-authored-by: Kiro Crew trailer added.

Local gates (all judged by exit code):

• isort / flake8 / mypy: clean
• Full pytest: 58665 passed. Two non-PR failures investigated and cleared:
  • test_history_coverage.py::…::test_a_genuine_append_still_invalidates_by_signature — pre-existing mtime-granularity flake on main (reproduced with pristine main content in the same env; two fast writes on tmpfs can land on the identical st_mtime, so the append does not advance the cache signature). Unrelated to this PR's diff; passed on the follow-up full run.
  • test_prepare_pr_prove.py::test_importing_prove_leaves_no_bytecode_in_the_checkout — self-pollution from an earlier interrupted local run leaving a stale __pycache__ in the checkout; passes after cleanup. Not a code defect.
• This PR's own tests (test_stage_result_off_loop.py, test_display_time_redaction.py) plus main's sibling off-loop suites (test_previous_result_read_off_loop.py, test_completion_result_read_off_loop.py, test_dashboard_chat.py): 651+28 passed.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

Push 4 (head cf93be9f): fixed the Backend Lint & Type Check failure — the baselined black gate (scripts/check_black_formatting.py) requires files NOT in .github/black-baseline.txt to be black-clean. The PR's new test/test_stage_result_off_loop.py is a new file (not baselined) and had 6 assertion-message closing parens black wants on their own line. Ran black on that one file only (12 insertions / 6 deletions, formatting only, zero semantic change); the other two files in the diff are baselined and untouched. Gate verified locally: black gate passed. The file's 4 tests still pass.

Note: pushes 2 and 3 were zero-diff amend refreshes recovering orphaned CI runner requests during today's runner-starvation window (same recovery pattern as PR #4570); no code changed in either.

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

BLOCKING -- src/kiro_crew/dashboard/chat_orchestrator.py:203 -- publication performs synchronous filesystem I/O on the event loop

os.replace(tmp_path, final_path)
Network-backed session directory -> _stage_loop -> _capture_stage_result -> synchronous replace stalls gateway tasks.
Anchor: no-blocking-call-on-event-loop
Fix: Revert the offending offload hunk.
[BLOCK-MERGE] 16e9d15
[GPT-REVIEWED] 16e9d15

[github-actions]

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

Premise-level review of 16e9d15a67d5727e5c291a817940f15b35499bd1 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. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need: the contract, the intent file, the patch, and the base chat_orchestrator.py, context_management.py, and the stop-flag plumbing. Composing the review.

Counts I ran on the base tree: tracker.stopped is consulted nowhere inside _stage_loop (grep tracker\.stopped → only the cancel handlers at chat_orchestrator.py:720 and chat_handlers.py:587); the loop's three stop checks (chat_orchestrator.py:235,482,558) test only slot._stopping; auto_run is a call-time parameter never re-read. So the "window" the rider closes predates the new await — in base, a Cancel during Go All never stopped the loop at all.

First-Principles-Verdict: CONCERNS

The stop re-check fixes a pre-existing cancel-is-ignored defect but is framed as closing a window the new await opened — and it patches one of four checkpoints.

What this change ships

Intent: stop stage-result persistence from stalling every other gateway task at each stage boundary — a FIX.

1. Stage-result mkdir+write moves to a worker thread — justified (Autopilot hardening: remaining P1-P3 findings after the P0 fixes #1783 P2; matches this module's existing _previous_result_paths/_completion_excerpts pattern).
2. Result lands via uuid temp file + atomic os.replace on the loop — declared rider; closes an orphan-writer race the offload itself creates; justified.
3. Cancelling a Go All plan now halts the run at the next stage boundary — declared rider, but symptom-level point patch (see Watch).
4. Failed publish unlinks the temp file — part of item 2.
5. New off-loop test suite plus two callers updated to await — the fix's own pins.

Watch

• Item 3's framing ("closing a window the new await itself opened") is contradicted by base: _stage_loop checks slot._stopping at 3 places (chat_orchestrator.py:235,482,558) and tracker.stopped at zero, while plan-action Cancel sets only tracker.stopped + slot._auto_run (chat_orchestrator.py:718-722) — so a cancel landing during await _run_chat or the subagent wait also advanced, in base. The cause is two stop channels with one listener. The rider happens to fix it at stage granularity, but a cancel during the subagent poll loop (line 479, slot._stopping only) still burns up to 15 minutes of polling before the new check halts it. The same-size general fix: consult tracker.stopped at the three existing checks.

Subtractions

• Drop the slot._stopping half of the new re-check: the top-of-loop check at chat_orchestrator.py:235 already breaks on it before any stage work; only tracker.stopped is load-bearing there.

[FIRST-PRINCIPLES-REVIEWED] 16e9d15

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 16e9d15a67d5727e5c291a817940f15b35499bd1 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real event-loop stall fixed at the right boundary — immutable data crosses to the worker, publication stays atomic and on-loop — proportionate and well-pinned.

Suggestions

• Orphan .stage_N_result.<uuid>.tmp files have no reaper: _reset_auto_run_for_new_plan (chat_title.py:618) globs only stage_*_result.md, so abandoned-worker residue accumulates in session dirs across re-plans — add .stage_*.tmp to that sweep.
• docs/system-specs/modules/autopilot.md documents _capture_stage_result and the loop's stop checkpoints; the new async signature, temp-write/publish scheme, and post-capture stop re-check belong in it in this same PR per the spec-sync rule.

[DESIGN-REVIEWED] 16e9d15

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

Push 5 (head 7ae44629): fixed GPT 5.6 round-1 BLOCKING (chat_orchestrator.py:582, cancel-during-offloaded-write advances the run).

Verification of the finding: legitimate, and introduced by this PR. The user-cancel path (chat_handlers.py) sets tracker.stopped + slot._auto_run = False, not slot._stopping; the loop's top-of-iteration check reads only slot._stopping, and the auto_run gate below the capture reads a call-time snapshot parameter, not the live slot flag. Pre-PR the capture was synchronous, so there was no suspension point between the loop's last stop check and the advance; the new await opened one.

Fix: after the awaited capture, re-check slot._stopping or tracker.stopped and break before advancing — exactly the reviewer's suggested shape, no scope beyond it.

Test: added test_stop_landing_during_the_offloaded_write_does_not_advance — drives the real _stage_loop with a 2-stage plan and lands the stop inside the stage-1 result write (mirrors the review scenario). Fail-before verified: with the guard removed the test fails ("stage 2 executed after the user's stop"), with it 5/5 pass. Full orchestrator suites (675 tests) + isort/flake8/mypy/black-gate all green locally.

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I have enough to decide. Let me verify there's no session-directory cleanup that would sweep the temp files (which would fully kill the candidate) and confirm the candidate's severity ceiling.

The candidate is a minor temp-file accumulation claim, self-rated "low" confidence. Applying Step 1's bar:

• The .stage_*_result.*.tmp orphan genuinely won't match the stage_*_result.md reset glob in chat_title.py:618, and no rmtree of the session dir exists. So the mechanism is real.
• But (a) requires a hard-cancel landing precisely inside the worker's write_text window (microseconds, small text), and (c) is accumulation of tiny dotfiles — no crash, data loss, corruption, or security boundary. The temp+replace pattern's whole purpose is to prevent corruption; leaving an orphan temp on a cancelled write is the explicitly-documented, accepted blast radius of every atomic-publish design.
• The realistic trigger frequency is speculative ("repeated cancel-during-write events"), and the harm is negligible. This does not clear 80+ confidence as a defect worth reporting, and it is nowhere near BLOCKING. Its fix would also live in an untouched file (_reset_auto_run_for_new_plan), which caps it at FINDING regardless.

Step 2: I reviewed the new os.replace/unlink error path (correct: cleans temp, re-raises, caller logs) and the new if slot._stopping or tracker.stopped: break re-check (correct: break skips the else completion arm and halts the run). No grounded new finding at 80+.

No findings.

[OPUS-REVIEWED] 16e9d15

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

Push 8 (head 16e9d15a): addressed Design Review CONCERNS (advisory, but point 1 is a real test decay).

1. test_completion_result_read_off_loop.py wrapper decay — confirmed exactly as described: the sync _capture_then_delete_stage_1 received an un-awaited coroutine from the now-async real_capture, so its unlink raised FileNotFoundError into the loop's except OSError and the test passed through the capture-failure path instead of the deleted-result-resilience path it pins. Wrapper is now async and awaits the real capture; verified the file's 10 tests pass with the unlink actually executing. Audited the file's other monkeypatch site (_capture_fails): it raises synchronously at call time, which the loop's except OSError catches identically for a sync or async callee — semantics intact, no change needed.
2. Description drift — already fixed in the previous cycle (PR body now documents the uuid-temp + os.replace publication scheme, the post-await stop re-check, and all six tests under "Review-driven hardening").

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]

⚠️ Escalation: review non-convergence — needs human adjudication

GPT 5.6 has now issued 4 consecutive BLOCKING rounds against this PR's core change (5 rounds total incl. the resolved R1), and rounds 2/3/5 each demanded "revert the offload hunk". The findings have reached a mutually exclusive pair of requirements, so further patching cannot converge:

Round	Finding	Disposition
R1	Cancel during offloaded write advances the run	✅ Fixed (push 5): post-await stop re-check, fail-before-verified test
R2	Cancelled await abandons a live writer → orphan overwrites reused stage file	✅ Fixed (push 6): shield + fence-wait
R3	The fence itself is outlived by slot deletion + key reuse	✅ Fixed (push 7): structural — worker writes uuid temp only; atomic os.replace publish on the loop thread after uncancelled return; orphan worker cannot reach the canonical file. GPT R4: ✅ zero blocking
R5	The loop-thread os.replace publish is itself "synchronous filesystem I/O on the event loop"	⛔ This escalation

Why R5 cannot be fixed without resurrecting R2/R3: the publish is the commit point. Move it into the worker (or any post-cancellation context) and an abandoned worker can again publish into a reused slot's canonical path — exactly the data-corruption class R2/R3 blocked on and R4 accepted the current design for. Keep it on the loop thread after an uncancelled await — the current design — and R5 objects to it. The two demands ("no syscall on the loop" and "publication must be impossible for an orphan") are jointly unsatisfiable at the commit point; some single atomic metadata syscall must anchor it somewhere.

Proportionality: os.replace is a metadata-only rename, the same class of syscall the loop already performs elsewhere (and orders of magnitude cheaper than the mkdir + payload write_text this PR removes from the loop, which is the issue #1783 bullet being fixed). The network-FS scenario invoked by R5 applies equally to code already on main.

Human decision requested — options: (a) override R5 via the AI-review override flow and merge on the current design (recommended; R4 already accepted it); (b) direct a specific alternative commit-point design; (c) revert the offload entirely per the reviewer (abandons #1783's P2 bullet).

State: CI 9/9 green on head 16e9d15a; Design ✅ PASS, Opus ✅ (head-1), FP 🟡 advisory answered (follow-up #4783), UX legitimately SKIPPED (backend-only). Author Leon preserved on the single commit with Co-authored-by: Kiro Crew. 8/10 pushes used.

[bolichen97]

This PR materially overlaps #3803 in dashboard/chat_orchestrator.py::_stage_loop and test_completion_result_read_off_loop.py. The invariants are complementary: this branch moves stage-result capture/write work off the event loop; #3803 moves orchestrator-config loading off-loop and adds stop gates.

Please coordinate a single replay of _stage_loop (preferably establish the config-read/stop-gate shape first, then apply the result-write offload) and keep both regression assertions. Neither PR should overwrite the other's cancellation/stop checks or reintroduce synchronous result-file I/O.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• This PR is OVERLAPPING with PR #7626. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #3771: REBASE. Both branches edit the same _stage_loop capture block for unrelated reasons. Sequence them (land one, rebase the other) and keep both invariants: the off-loop write and the uncaptured-round rollback. Files: src/kiro_crew/dashboard/chat_orchestrator.py, test/test_dashboard_chat.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Closing as superseded by merged #8618.

The audit compared this branch's full diff against origin/main and #8618 (src/kiro_crew/dashboard/chat_orchestrator.py, test/test_completion_result_read_off_loop.py, test/test_display_time_redaction.py): #8618 landed the same off-loop split for stage-result writes (message walk stays on the loop, mkdir + write_text go to a worker), additionally moved redact_exfiltration_urls / redact_credentials off-loop, and ships an equivalent test file (test/test_stage_result_write_off_loop.py). It also deleted the _capture_stage_result this branch rewrites, so the diff no longer applies to any function on main (branch is ~2500 commits behind and conflicting).

One piece #8618 did not take is worth its own small PR against current main: cancellation-safe publication (uuid temp file + abandoned flag + atomic os.replace). If you want to pursue that, please open it fresh rather than rebasing this branch. Thanks for the fix -- it got there first, the merged version just happened to land via another route.