fix(taskrunner): move the spec and plan-directory I/O off the gateway loop

[leonlaiyc]

Problem / Motivation

AUTOSDE.yaml's no-blocking-call-on-event-loop rule (blocking: true,
file-patterns: src/kiro_crew/**/*.py) names "large synchronous file IO or
filesystem walks" as calls that must not run on the gateway loop, and closes
with "When in doubt, offload — a leaked worker thread is survivable; a frozen
loop is not." Two task-runner handlers do their filesystem work inline anyway.

api_taskrunner_start:

if not spec_path.startswith("__inline__:"):
    resolved = Path(spec_path).resolve()                    # caller-chosen path
    if ".." in Path(spec_path).parts or not resolved.is_file():
        ...
...
fpath.parent.mkdir(parents=True, exist_ok=True)
fpath.write_text(content, encoding="utf-8")                 # caller-supplied body
...
created_spec.unlink(missing_ok=True)                        # rejected-start cleanup

api_taskrunner_from_chat:

while True:
    task_dir = state.task_runner._work_dir / f"plan_{uuid.uuid4().hex[:8]}"
    try:
        task_dir.mkdir(parents=True, exist_ok=False)
        break
    except FileExistsError:
        continue
...
task_dir.rmdir()                                            # rejected-plan rollback

Five blocking syscalls, all on the loop, and the two riskiest take their input
straight from the request body: write_text commits an arbitrary,
size-unbounded inline spec, and resolve() / is_file() run against whatever
path the caller named — a UNC path to a disconnected share resolves at the
mount's timeout, not at RAM speed.

Why it matters

The rule states the consequence directly: "a single blocking call there freezes
every task — the user's chat turn AND the liveness heartbeat — until the
watchdog kills the process and the supervisor respawns into the same condition
(a crash loop). This has caused multiple production wedges."

Starting a task from a spec is an ordinary dashboard action, so this is not an
exotic path. The inline-spec write scales with a body the caller controls, and
the path guard's cost is set by a filesystem the caller points at. The rollback
rmdir and unlink are individually small, but they sit on the failure path,
which is exactly where a wedged volume is most likely to be the reason the
operation failed in the first place.

What changed (motivation → approach → change)

Root cause is simply that the offload was never applied here — the module knows
the rule and cites it by name in _gate_auto_approve, which already routes its
synchronous SEL flush through asyncio.to_thread. Each site moves to a worker,
grouped so that a sequence costs one hop rather than one hop per call:

• _validate_spec_path(raw) — the resolve, the .. check, the is_file
  stat and the is_sensitive_path denylist move into one function returning
  (validated_path, failure_code) — the handler maps the code to its literal
  error response. The validation logic is unchanged; what changes is
  that it runs in a worker and the handler receives only a path that passed.
  That is the same property the inline comment asked for ("no gap where
  spec_path could differ from what was checked"), and grouping strengthens it:
  the guard and the value it produces can no longer be separated at all, and no
  await lands between them.
• _write_inline_spec(work_dir, content) — the name mint, the mkdir and
  the write_text in one hop.
• _make_plan_dir(work_dir) — the collision-retry loop stays with the
  mkdir it retries, so N collisions still cost one hop instead of N.
• The rejected-start unlink and the rejected-plan rmdir are awaited through
  asyncio.to_thread; both keep their existing except OSError best-effort
  handling, and the from_chat rollback still re-raises the original
  ValueError so the handler's 400 is unchanged.

Behaviour deltas (deliberate, driven by CI gates and review):

• The two spec-path error responses now carry machine-readable code fields
  (invalid_spec_path / access_denied), per the error-code contract gate and
  the existing convention (handlers/prompts.py); error-code-baseline.json
  re-snapshotted for the legitimate drop (taskrunner missing_code 43 -> 41).
• A request cancelled mid-flight now removes the spec file / plan directory it
  created (previously orphaned): the /start cleanup catches BaseException
  (mirroring the from_chat rollback), so a cancel during start_background
  cleans up the inline spec before the cancellation propagates.
  Status codes, response ordering, and best-effort cleanup semantics are
  otherwise unchanged.

Tests

New TestSpecIoRunsOffTheEventLoop in
test/test_handlers_taskrunner_coverage.py. Each test spies on the real
pathlib method (filtered to the file or directory the handler owns, so an
unrelated call cannot decide the result), records threading.get_ident(), and
asserts no call landed on the test coroutine's thread — the property holds
however the handler reaches the filesystem, and does not depend on the shape of
the fix:

• test_inline_spec_write_runs_off_the_event_loop_thread (write_text, also
  asserts the file's contents so an offload that lost the write fails)
• test_spec_path_guard_runs_off_the_event_loop_thread (is_file, driven with
  a real spec file so the guard passes and the handler returns 200)
• test_rejected_start_removes_the_inline_spec_off_the_loop (unlink, with
  start_background raising)
• test_from_chat_plan_dir_mkdir_runs_off_the_event_loop_thread (mkdir)
• test_from_chat_rollback_rmdir_runs_off_the_event_loop_thread (rmdir, with
  update_plan raising ValueError; still asserts the 400)
• test_cancelled_inline_write_does_not_orphan_spec (cancel during the
  shielded write; the drained worker's file is removed, no TASK_*.md orphan)
• test_cancelled_start_backgrounding_does_not_orphan_spec (cancel after the
  write, during start_background; the BaseException cleanup removes the
  spec before the cancel propagates)
• test_cancelled_plan_claim_does_not_orphan_directory (cancel during the
  plan-directory claim; no plan_* orphan, run map left empty)

Red-before, measured against pristine origin/main production code
(ad0825392) with the new tests in place — 5 failed / 100 passed, every
failure the ident comparison, i.e. the defect itself and not a fixture or
environment artifact:

assert ([34652] and False)          # inline spec write_text
assert ([43608] and False)          # spec path is_file
assert ([9724]  and False)          # rejected-start unlink
assert ([31564, 31564] and False)   # plan dir mkdir
assert ([39976] and False)          # rollback rmdir

Green-after: 105 passed in that module. Blast radius: 451 passed / 1
skipped across test_handlers_taskrunner_coverage.py, test_taskrunner.py,
test_taskrunner_coverage.py, test_taskrunner_autoapprove.py,
test_taskrunner_atomic_persistence.py and test_taskrunner_v2_scenarios.py.

flake8, isort and mypy are clean on both files. Both are on
.github/black-baseline.txt and keep the same pre-existing hunk count before
and after; the added lines are black-clean.

Manual verification

N/A — unit coverage sufficient: the property is which thread a syscall runs on,
observed directly at the pathlib call. Reproducing the stall by hand needs a
deliberately wedged volume, which the thread assertion pins without one.

Related Issues

Self-reported while auditing AUTOSDE.yaml's
no-blocking-call-on-event-loop rule against the dashboard handlers. No
separate issue was filed.

Checklist

• At most two commits (one is the norm), 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)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

Pattern harvest

Rule candidate: an aiohttp handler that creates a filesystem side effect it owns (temp spec file, plan directory) must (1) run the blocking create off the event loop (asyncio.to_thread), and (2) treat cancellation as an ownership hazard — shield the worker, drain it to completion on CancelledError, then remove the created artifact before re-raising, or a cancelled request orphans the artifact with no owner left holding its path. Grep candidates: write_text|mkdir|unlink|rmdir directly inside async def api_* handlers.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]: 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: Merge conflict needs a rebase onto main; backend shards fail deterministically on test/test_error_code_contract.py (the relocated error responses in _validate_spec_path need machine-readable code fields, and the contract baseline needs re-snapshotting per the ratchet's procedure); Coverage Gate is fail-closed behind the red shards.

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

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]

Drive-to-green update: rebased and fixed the three assessed blockers. Head is now d636183b8 (was d3405d2d5).

1. Merge conflict → rebased onto origin/main (992498caf)

• The branch was CONFLICTING (created 2026-08-26; main moved). Rebased as a cherry-pick of the single content commit onto main tip — authorship preserved (Leon remains author of the content commit).
• src/kiro_crew/dashboard/handlers/taskrunner.py applied cleanly; one ADD/ADD conflict at the tail of test/test_handlers_taskrunner_coverage.py between main's new TestNonObjectBodiesAcrossConvertedHandlers (issue dashboard: consolidate the two JSON-object body-guard helpers, then sweep the ~85 unshaped request.json() handlers #5587) and this PR's TestSpecIoRunsOffTheEventLoop. Resolved by keeping both classes — zero changes to either side's test logic. The PR's off-loop helper design (_validate_spec_path, _write_inline_spec, _make_plan_dir, _drain_worker, _remove_owned_path, _materialize_inline_spec, _claim_plan_dir + shield/drain cancellation) is untouched.

2. test_no_new_error_response_without_a_code → literal, compliant responses (commit d636183b8)

• The refactor had folded the two spec-path error responses into one variable-status site (web.json_response({"error": error}, status=status)), which the contract scanner buckets as a new dynamic_status finding against a baseline of 0 for that file.
• Fix follows the contract test's own guidance and the repo convention (mirrors handlers/prompts.py): _validate_spec_path now returns a machine-readable failure code and the handler emits two fully literal responses — {"error": "invalid spec path", "code": "invalid_spec_path"} (400) and {"error": "access denied", "code": "access_denied"} (403). Validation still runs off-loop via asyncio.to_thread; response prose is unchanged.

3. test_baseline_is_not_stale → baseline re-snapshotted

• Ran the documented procedure python test/test_error_code_contract.py --update. Result: dashboard/handlers/taskrunner.py missing_code 43 → 41 — a legitimate drop only (totals 1204 → 1202; opaque_body/dynamic_status unchanged; no number raised).

Local gates (all green)

• isort --check-only ✅ · flake8 ✅ · mypy src/kiro_crew/ ✅ (1277 files, no issues)
• test/test_error_code_contract.py 6/6 ✅ · test/test_handlers_taskrunner_coverage.py 174/174 ✅ (both the PR's new class and main's dashboard: consolidate the two JSON-object body-guard helpers, then sweep the ~85 unshaped request.json() handlers #5587 class)
• Full suite: 82,670 passed; the 98 failures are a strict subset of the 99 failures a pristine-main control worktree produces on the same host (host-env issues: AF_UNIX path too long etc.) — zero regressions from this PR.

Coverage Gate should clear on its own once the backend shards go green (fail-closed behind them; no .github/** touched).

Now monitoring CI; will address any review-bot findings next.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]

PR Hygiene fix (zero-push): the check requires fix-type PRs to carry a ## Pattern harvest section with a Rule candidate: line. Appended it to the PR body (everything else in the body untouched) via a REST body PATCH — the edited event re-runs the check without a new head, so the in-flight CI wave on d636183b8 stays valid. No code change.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]

Round 3 pushed: f2343e3ce → ec329fc8d. Dispositions for the round-2 verdicts:

GPT 5.6 — 🔴 BLOCKING: "cancellation deletes a retained run's spec" — ADOPTED (finding verified real; fixed at the root instead of the literal revert)

• Verified against taskrunner.py source: start_background registers the run placeholder inside its admission lock and then awaits _workflow_begin(...) outside its rollback try (the rollback covers only the _apersist_runs hop). A cancel landing there retains the run while the round-2 handler cleanup unlinked its spec — real corruption, exactly as flagged.
• The demanded fix ("restore Exception-only cleanup") would have re-opened the First Principles round-1 finding (cancel during start_background before registration orphans the spec) and left the same corruption reachable on the plain-Exception path: a _workflow_begin failure after registration also retains the run, and the pre-PR code unlinked the spec there too — the hazard predates the BaseException widening.
• Fix: cleanup is now ownership-gated. _spec_retained_by_run(state, created_spec) checks whether any registered run references the created spec; the handler removes the spec only when it is still the sole owner. Covers both the cancel path and the pre-existing Exception path.
• Mutation-verified in both directions: dropping the gate fails the new test_cancelled_start_never_unlinks_a_spec_a_registered_run_retains (registering start_background stub + cancel → spec must survive); narrowing BaseException back to Exception fails test_cancelled_start_backgrounding_does_not_orphan_spec (non-registering cancel → spec must be removed).

First Principles — 🟡 CONCERNS (advisory)

• Watch: _drain_worker is a third spelling of the mcp.py drain loop, and diverges (swallows worker exceptions where _offload_config_write propagates) — ACKNOWLEDGED, DECLINED as an in-PR change (scope): the divergence is deliberate in this PR's design — _drain_worker's contract is settle-don't-report (its callers inspect worker.result() themselves afterward and decide cleanup vs. propagate), while _offload_config_write is the terminal awaiter so it must propagate. Consolidating the three spellings is legitimate follow-up refactoring; replacing the contributor's mechanism is outside drive-to-green's mandate. Opus round 1 independently verified the drain/result/re-raise chain.
• Watch: 136 raw Path I/O hits across 28 sibling handler files — ACKNOWLEDGED: deferred by the PR's per-file scope, matching the repo's ratchet convention (as the review itself notes).
• Subtraction: fold _remove_owned_path into the _offload_config_write spelling — DECLINED (scope): same rationale as above and as round 2; the compensating-delete capability the review agrees is new is exactly what these helpers carry.

Design — ✅ PASS round 2 (Watch adopted in round 2: description rewritten). Opus — ✅ no findings round 2.

Gates on ec329fc8d (local, pre-push): isort/flake8/mypy clean (1279 files); test_handlers_taskrunner_coverage.py 176/176 (new retained-run test included); test_error_code_contract.py 6/6; full suite 82,993 passed, fail set identical to the pristine-main control on this host (99=99, zero PR regressions).

Commit count stays at 2. Monitoring the round-3 wave.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]

First Principles round-3 dispositions (ec329fc8d, CONCERNS — advisory; GPT/Opus/Design all ✅ on this head):

• Watch: the description's _validate_spec_path signature text says (validated_path, error, status) but the helper returns a two-tuple — ADOPTED (zero-push): body PATCHed to (validated_path, failure_code) with the handler mapping the code to its literal response. Accurate to the shipped code; no head change, the converged green wave and all four verdicts on ec329fc8d stay valid.
• Watch: items 6–7 (the start_background cancel-orphan fix + _spec_retained_by_run probe) are a second fix riding along and "could ship separately" — ACKNOWLEDGED, KEEPING: the review itself grants they are declared-deliberate and non-blocking. Splitting them out now would revert the adopted resolution of GPT's round-2 BLOCKING (retained-run spec deletion) and FP's own round-1 finding (the uncovered start_background window) — the two reviews jointly forced exactly this shape, and un-shipping it to satisfy a size preference would re-open both.
• Subtraction: replace _remove_owned_path's create_task/shield/drain with a bare await asyncio.to_thread(op) since "a leaked worker thread is survivable" — DECLINED (scope, consistent with rounds 1–3): it rewrites the contributor's cancellation-ownership mechanism, which is functionally correct (Opus verified the drain/result/re-raise chain; round 3 finds no defect in it either). Whether the drain is worth ~30 lines vs. an abandoned thread is a style/size judgment for the author or a follow-up refactor, not a green-blocker. Notably this round's evidence header confirms "no existing shield/drain helper is duplicated", retiring round 2's duplication premise.

All four review lanes have now converged on ec329fc8d with zero blocking findings. Awaiting the PR Readiness rollup.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]

✅ Drive-to-green complete — PR #5987 is review-ready.

Final state on head ec329fc8d: PR Readiness = success (readiness: passed), all check-runs green, mergeable=true, and all four review lanes resolved — GPT 5.6 ✅ no findings, Opus 4.8 ✅ no findings, Design ✅ PASS, First Principles 🟡 CONCERNS (advisory; every item adopted, answered, or dispositioned in the round comments above).

What this drive did (full detail in the per-round comments):

1. Rebased the branch twice onto moving main (final base 6ade33314), resolving the test-file ADD/ADD conflict (kept both main's dashboard: consolidate the two JSON-object body-guard helpers, then sweep the ~85 unshaped request.json() handlers #5587 class and the PR's off-loop class) and the error-code-baseline.json collision — the PR's off-loop helper design is fully preserved, Leon remains author of the content commit.
2. Error-code contract (the original red): _validate_spec_path returns a machine-readable failure code; the handler emits two literal compliant responses (invalid_spec_path 400 / access_denied 403, matching prompts.py); baseline re-snapshotted per the documented procedure (legitimate drop only, 43 → 41).
3. PR Hygiene: added the required ## Pattern harvest section (zero-push body PATCH).
4. Review findings adopted: First Principles' uncovered start_background cancel window (cleanup widened to BaseException, mirroring from_chat), then GPT's round-2 BLOCKING on that very fix (a cancel after run registration must not unlink a retained run's spec) resolved with the ownership gate _spec_retained_by_run — which also closes the same pre-existing hole on the Exception path. Both directions mutation-verified; 2 new tests.
5. Description corrected twice (behaviour-delta declaration + helper signature) so the body matches the shipped diff.

Verification: isort/flake8/mypy clean; test_handlers_taskrunner_coverage.py 176/176; contract suite 6/6; full local suite 82,993 passed with the fail set identical to a pristine-main control on the same host (zero PR regressions), re-verified after every amend.

Handoff to maintainers: this PR needs one approval from a maintainer other than bolichen97 (branch protection counts require_last_push_approval, and the drive's pushes were made under that login). Commits: 2 (Leon's content commit + one fix commit with Co-authored-by: Kiro Crew). The drive-to-green label stays on for the cleanup cron.

Thanks @leonlaiyc for the contribution — the off-loop design held up through three review rounds unchanged.

[bolichen97]

Rebased onto main 9af9543b by a maintainer as part of the 2026-09-08 open-PR audit.

Conflicts and resolutions:

• error-code-baseline.json: took main's snapshot and re-ran python test/test_error_code_contract.py --update, so dashboard/handlers/taskrunner.py is 43 -> 41 as this PR intends, on main's current totals.
• test/test_handlers_taskrunner_coverage.py: auto-merged. Main replaced the local _Payload class with body_stream_helpers.BodyStreamPayload (refactor(dashboard): sweep tranche 3 onto read_bounded_json (#5587) #7308); your new TestSpecIoRunsOffTheEventLoop class appends cleanly on top. No behaviour change.

Gates run locally on the changed files: black, isort, flake8, and pytest test/test_handlers_taskrunner_coverage.py test/test_error_code_contract.py (182 passed).

Please review the resolution. A maintainer pushed last, so under the repo's last-push rule a second approver is needed. Reply if anything looks wrong.