fix(dashboard): offload eager spawn's agent-binding config load by leonlaiyc · Pull Request #4118 · kirodotdev/KiroCrew

leonlaiyc · 2026-08-17T12:38:38Z

Problem / Motivation

_eager_spawn in src/kiro_crew/dashboard/chat_runner.py resolves the slot's
agent bindings before the speculative handshake, and that read is synchronous:

_bound = (slot.agent, slot.model, slot.project, slot.reasoning_effort)
...
try:
    cfg = KiroCrewConfig.load()                      # on the gateway loop
    bindings = resolve_agent_bindings(cfg, slot.agent or None)

KiroCrewConfig.load() is not a cheap accessor. It resolves the config path,
exists()/read_text()/json.loads() on config.json, is_file()/stat() and
deep-merges any config.local.json overlay, then runs the full jsonschema
validation. None of that yields, so the loop is occupied for the whole read.

Being accurate about the cost: the loader keeps a fingerprint cache, so a warm
call short-circuits. The blocking read is the cold path.

Correcting this section against the First Principles review of a415e3276, which
is right: _eager_spawn is created only by schedule_eager_spawn, and that
function runs its own bare KiroCrewConfig.load() on the loop at
chat_runner.py:3229 to test session.eager_spawn before creating the task. So on
a cold gateway the loop pays the cold read there, roughly a debounce ahead of the
load this PR offloads, and the offloaded call then usually hits the warm
fingerprint cache. The cold case that reaches this call site is narrower than the
section originally claimed: a config write landing inside the debounce window.

What this PR still buys, stated honestly: the offloaded call is off the loop
whether warm or cold, so the loop stops paying even the warm path's stat pass and
stops being exposed to the cold case above. What it does not do is remove the
cold read from the loop, because the schedule_eager_spawn:3229 load is the one
that lands first. That load is a separate defect and is not fixed here -- see
Scope.

Why it matters

_eager_spawn is speculative. It exists only to make a slot's first real
message faster; nothing depends on its result, and it is already debounced and
capped by _eager_spawn_sem. So an optional latency optimisation was doing cold
filesystem and schema work on the single loop that every live session, every
WebSocket broadcast and every HTTP handler shares — the one place where the work
has no claim on the loop at all.

It is user-triggered rather than rare: schedule_eager_spawn re-arms on slot
create, agent switch, project set and slot focus.

No latency figure is claimed; none was measured.

What changed (motivation → approach → change)

Motivation — take the filesystem and schema work off the shared loop without
altering a single handshake semantic.

Approach — not a new idiom. await asyncio.to_thread(KiroCrewConfig.load) is
already the established form on more than a dozen async paths, several of them in
this package, and chat_runner.py itself already carries eight asyncio.to_thread
offloads — one of them justified in-comment as "_run_chat shares the single
gateway event loop with every other session". This call site is the outlier.

Change — the load, and only the load, moves to a worker:

worker:     KiroCrewConfig.load()
loop:       resolve_agent_bindings(cfg, slot.agent or None)
            read slot bindings, acquire the session, revalidate

No slot, session, DashboardState or tracker crosses the boundary. No new config
executor, no caching layer, no change to reload semantics, no change to any
default. The existing broad except Exception is kept: an exception raised inside
to_thread still propagates through the await into the same handler.

Why this is safe: the suspension lands inside an existing envelope

The new await widens a window that already exists rather than opening a new one.
_eager_spawn already maintains a snapshot → handshake → revalidation envelope,
and the load sits inside it:

_bound = (slot.agent, slot.model, slot.project, slot.reasoning_effort)
    ↓
await asyncio.to_thread(KiroCrewConfig.load)      <- the new suspension
    ↓
resolve bindings
    ↓
await sessions.get_or_create(...)
    ↓
not is_new              -> leave the winning session alone
slot identity changed   -> remove the session this task created
bindings != _bound      -> remove the session this task created

Every action newly able to land during the load — slot deletion or replacement,
an agent/model/project/effort switch, another creator winning the same key — is
therefore revalidated after the handshake by guards that predate this change.
That is why no generation counter, no new slot field and no lock is added here.

Cancellation needs plumbing, but not new plumbing. Correcting this against the GPT
review of a415e3276, which is right that "the load is read-only" is false:
KiroCrewConfig.load publishes three process-global snapshots on every call
(mcp.extra_path_dirs, the agent alias table, the autocompact threshold) and its
write-back migration can rewrite config.json.

An abandoned worker is still acceptable, for a narrower reason than the one
originally given. The autocompact publish is ordered on a ticket drawn before the
read, so a load that began earlier cannot overwrite a newer one -- the loader
designed for exactly this. The other two are idempotent in-memory rebinds of what
was just read, so republishing an unchanged config is a no-op.

The residue, stated rather than hidden: those two publishes carry no ordering of
their own, so a worker that finishes late can republish a value a newer load has
already superseded. That is a property of concurrent loads in general and not of
this call site -- main already loads off-loop at the stop-hook nudge-cap site --
so it is not introduced here and is not fixed here.

Tests

test/test_eager_spawn_config_load_off_loop.py:

test_config_load_runs_off_the_event_loop — the fail-before. Wraps the
loader _eager_spawn actually calls with a delegating wrapper that records
threading.get_ident(), invalidates the fingerprint cache so the recorded
thread genuinely does the disk work, drives the real _eager_spawn, and
compares against the loop thread. On an unfixed tree:
```
AssertionError: KiroCrewConfig.load() ran on the event loop thread (23964);
the gateway is blocked for the whole read, merge and jsonschema validation
```
No timing assertion, no sleep.
test_slot_replaced_during_the_load_leaves_no_orphan_session and
test_binding_switch_during_the_load_removes_the_stale_session —
preservation coverage for the new window. A threading.Event-gated loader is
held suspended, the lifecycle action is fired on the loop while it is
suspended, and the existing guards are shown to still tear the session down.
These are coverage for the new await, so they are not expected to fail on a
pristine tree — only the thread-identity test does that. The event waits are
bounded purely as hang guards; nothing asserts on elapsed time.

Local: test_eager_spawn.py, test_eager_spawn_config_load_off_loop.py,
test_chat_runner_coverage.py, test_active_turn_session_key.py,
test_chat_slot_project.py, test_chat_slot_reasoning_effort.py,
test_chat_slot_mode.py, test_dashboard_chat.py — 899 collected, all green.
The wider set of 74 test files that reference chat_runner is 4487 passed /
76 skipped, with 22 failures that reproduce identically with this commit's
production change stashed: 8 × WinError 1314 (symlink privilege) and 14 in
test_dashboard_file_io.py, both pre-existing on this Windows host.

flake8, isort --check-only and mypy --platform linux clean on both changed
files; git diff --check clean.

Scope

One call site. KiroCrewConfig.load appears at seven sites in this module: one is
fixed here, one already runs off-loop (the stop-hook nudge cap), and five bare
on-loop loads remain. The First Principles review of a415e3276 is right that this
section previously named only two of the five, so here they all are:

site (line at `8161898af`)	in scope?
`schedule_eager_spawn` (3229)	no -- see below
`_empty_auto_continue_enabled` (295)	no
`_start_next_queued_turn` (4224)	no -- has no suspension point today
`_run_chat` -> nested `_queue_recovery` (5150)	no
`_run_chat` -> nested `_queue_recovery` (5391)	no

schedule_eager_spawn:3229 is the consequential one, because it is this feature's
own first load and it is the one that actually pays the cold read on the loop.
First Principles proposes deleting it and gating session.eager_spawn inside
_eager_spawn from the already-offloaded cfg, which is the right shape: the
feature would then load config once, off-loop.

It is deliberately not done in this PR. This is an adoption rebase of a
contributor's change onto current main, and restructuring a second function's
control flow -- moving the feature's enablement gate from before task creation to
after it, which changes when a disabled feature stops creating tasks -- is a
different change needing its own argument and its own tests. Folding it in here
would trade this PR's short, checkable argument for a much longer one, which is the
same reason the other four are excluded. It should be its own issue and PR, and the
Pattern harvest gate entry below would surface all five as a bounded list.

Pattern harvest

Rule candidate: extend scripts/check_sync_io_in_async.py with a small allowlist of
known-blocking FIRST-PARTY helpers, KiroCrewConfig.load first among them.

The gate that exists for exactly this defect class did not catch it, and the reason
generalises. check_sync_io_in_async.py matches <module-alias>.<attr>(...) for the
stdlib and client IO modules (os, pathlib, shutil, subprocess, requests,
httpx, sqlite3), plus DB methods and HTTP verbs. Every blocking call here is real
-- path resolution, exists(), read_text(), json.loads(), is_file(), stat(),
a deep merge and full jsonschema validation -- but all of it sits one frame down,
behind a first-party synchronous facade, so the AST scan sees only
KiroCrewConfig.load() and reports nothing. The gate passes on this file both before
and after the fix.

So the miss is structural, not an oversight in the rule's implementation: any
synchronous project-local function that performs IO internally is invisible to it,
and KiroCrewConfig.load is the highest-traffic instance of that shape in this tree.
An allowlist keyed on the callee name would have flagged this call site at the moment
it was written.

Two adjacent facts worth recording for whoever picks that up. main already carries
await asyncio.to_thread(KiroCrewConfig.load) at another call site in this same
module, so the fixed form is the one the file had already settled on -- the defect was
a lone holdout, which is the signature of a rule that should be mechanical rather than
remembered. And five bare on-loop loads remain in chat_runner.py, inventoried
under Scope above and all deliberately out of scope here; a gate entry would surface
them as a bounded, reviewable list instead of leaving them to be rediscovered one
review at a time -- which is how schedule_eager_spawn:3229 surfaced on this PR.

Fixes #4117

bolichen97 · 2026-08-21T20:29:57Z

Hi @leonlaiyc, a maintainer nudge on this one: it is carrying the readiness: action required label and has had no activity for about 4 days, so it is not moving toward merge.

Current state:

Base: 356 commits behind main
Merge state: mergeable, but the readiness gate is still red
Red signals:
- Backend Tests (Windows) (2) (cancelled)
- PR Readiness

Could you rebase onto the latest main and push a fix?

git fetch upstream           # or: git fetch origin, if this branch lives here
git rebase upstream/main
# resolve any conflicts, run the local gates below, then update the branch with
# a force-with-lease so the rebase lands without clobbering anyone else's work

The local gates to run before updating the branch:

black src/kiro_crew test && isort src/kiro_crew test
flake8 src/kiro_crew test && mypy src/kiro_crew
python -m pytest
cd website && npm run build && npm run test

A good part of this branch's redness is likely stale rather than a real defect: it predates a lot of what is now on main, and several of these gates (the Coverage Gate, the Windows shards, the AI review lanes) have changed since the last run here. A rebase alone often clears them. If something still fails afterwards and you believe it is a false positive, say so in a comment and we will take a look. If the change is no longer needed, feel free to close the PR.

Every config.json update in dashboard/handlers/memory.py reads the file, changes one key and writes it all back on the event loop. The read, the JSON parse, and write_config_atomically -- a tmp-file write plus a rename, which can fsync -- all block, stalling every other session while they run. Three sites do it: api_memory_settings, _write_embed_model_config, and _set_migrated, which runs on EVERY boot while migrated is false and so lands the stall exactly when the gateway is bringing sessions up. This is the class the repo has been closing site by site (kirodotdev#4118, kirodotdev#3803, kirodotdev#4550), and kirodotdev#4946's review named this module. The whole transaction crosses over, never just the read. Offloading the read alone would leave the write on the loop and insert a suspension point between the read and the write-back while the file is unguarded on disk: an external editor, a CLI command or another process landing in that gap would be silently overwritten by a write derived from state nobody re-checked. That gap is zero today because the sequence is synchronous, and it stays zero because the worker performs the whole thing without yielding. The existing per-config lock is held across the hop, so two coroutines still cannot interleave. Two of the three sites also hand-rolled a reader this module already imports. read_config_for_update is the documented companion to write_config_atomically with 27 call sites, and api_memory_settings uses it 200 lines above; _set_migrated and _write_embed_model_config instead caught Exception around json.loads. The helper additionally refuses a non-object top level, where the hand-rolled version accepted a list and then raised AttributeError from setdefault -- a crash where a fail-closed refusal was intended. ConfigReadError is not swallowed by the helper: what to tell the user differs per site, and each keeps exactly the behaviour it had -- skip and retry next boot, raise ValueError, or answer 500 config_unreadable. api_memory_settings now validates its body before the transaction. None of that reads the config, and a 400 previously took the lock and abandoned it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

_write_env_updates stats and reads the whole .env, re-parses it line by line, then creates a 0600 temp file, chmods it, writes and renames. All synchronous file I/O, and six async channel config-save handlers call it. Three already reached it through asyncio.to_thread -- telegram, teams, wecom -- and three called it inline on the gateway loop: slack, discord and webex, stalling every other session for the duration of a token save. So this is not a missing convention but an existing one applied to half the call sites. messaging.py already uses asyncio.to_thread 29 times, and with three siblings doing it correctly nothing in the file said which half was right, or stopped the next channel from copying the wrong one. It is the class the repo has been closing site by site (kirodotdev#4118, kirodotdev#3803, kirodotdev#4550), and kirodotdev#4946's review named this module. The WHOLE call is offloaded, never a part of it: the read-modify-write is one transaction, and a suspension point between the read and the rename would let a concurrent writer's keys be dropped by a write derived from lines nobody re-read. Keeping _write_env_updates one synchronous function on one worker preserves that without depending on the caller, which is now said on the function itself so the next channel inherits the reason and not just the shape. The regression pins all six channels rather than the three that moved, since the defect was the split and not any one site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Every config.json update in dashboard/handlers/memory.py reads the file, changes one key and writes it all back on the event loop. The read, the JSON parse, and write_config_atomically -- a tmp-file write plus a rename, which can fsync -- all block, stalling every other session while they run. Three sites do it: api_memory_settings, _write_embed_model_config, and _set_migrated, which runs on EVERY boot while migrated is false and so lands the stall exactly when the gateway is bringing sessions up. This is the class the repo has been closing site by site (kirodotdev#4118, kirodotdev#3803, kirodotdev#4550), and The whole transaction crosses over, never just the read. Offloading the read alone would leave the write on the loop and insert a suspension point between the read and the write-back while the file is unguarded on disk: an external editor, a CLI command or another process landing in that gap would be silently overwritten by a write derived from state nobody re-checked. That gap is zero today because the sequence is synchronous, and it stays zero because the worker performs the whole thing without yielding. The existing per-config lock is held across the hop, so two coroutines still cannot interleave. Two of the three sites also hand-rolled a reader this module already imports. read_config_for_update is the documented companion to write_config_atomically with 27 call sites, and api_memory_settings uses it 200 lines above; _set_migrated and _write_embed_model_config instead caught Exception around json.loads. The helper additionally refuses a non-object top level, where the hand-rolled version accepted a list and then raised AttributeError from setdefault -- a crash where a fail-closed refusal was intended. ConfigReadError is not swallowed by the helper: what to tell the user differs per site, and each keeps exactly the behaviour it had -- skip and retry next boot, raise ValueError, or answer 500 config_unreadable. api_memory_settings now validates its body before the transaction. None of that reads the config, and a 400 previously took the lock and abandoned it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

chenmingwei23 · 2026-09-01T23:35:14Z

Round 2 verdicts on ab9d703bf: GPT 5.6 and Opus 4.8 both No findings (the
read-only correction cleared GPT's round-1 finding), Design PASS, UX skipped-no-UI,
First Principles CONCERNS (advisory) with two subtractions. Head is now
0bea21daf.

First Principles subtraction 1: the false claim in the test module docstring

Accepted, and fixed. This one is mine and it is a fair catch.

Round 2 corrected the "the load is read-only" claim in the call-site comment, the
commit message and the PR body -- and missed the new test module's docstring, which
still ended with "the load is read-only and its result is simply abandoned, so no
cancellation plumbing is needed for it." So the diff contradicted itself: the
production comment said the load is NOT read-only while the test docstring alongside
it said it is.

Fixed by keeping one truth, as the lane asked, and by not duplicating the argument:

A worker left running by a cancelled eager task is acceptable, but not because
the load is read-only -- it is not. The reasoning lives in the call-site comment
in ``_eager_spawn`` and is deliberately not restated here: it turns on which of
the loader's process-global publishes are ticket-ordered, and two copies of that
argument would drift. Nothing in this module tests it.

Restating the reasoning in the docstring is what let the two copies diverge in the
first place, so the docstring now points at the single source of truth instead of
paraphrasing it.

First Principles subtraction 2: delete the bare load at `schedule_eager_spawn`

Re-raised from round 1. Disposition unchanged: declined for this PR, tracked as
#7734.

The lane adds one new supporting fact this round -- that every caller of
schedule_eager_spawn except ws.py:492 discards its return value. That is true
and it does retire one objection (nothing downstream depends on the returned task).
It does not reach the three that carry the decision, which are about behaviour
rather than the return value:

The session.eager_spawn gate moves from before task creation to after
it. Today, with the feature disabled, schedule_eager_spawn creates no task and
cancels no predecessor. Afterwards it would create a task per trigger signal that
wakes, loads config off-loop, and returns -- cheap, but a different thing to
observe, and it changes what slot._eager_spawn_task holds while the feature is
off.
The prev.cancel() branch runs after the gate today only because the gate sits
above it. Moving the gate reorders cancellation against enablement.
_eager_spawn currently reads a config-load failure as "resolve failed, tear
down". It would additionally have to read it as "feature disabled, do nothing",
and those two want distinguishing.

Each wants its own test, which is the argument for a separate PR rather than for
never. #7734 carries the full analysis and credits this lane.

Line numbers in the PR body were stale, and are corrected

Round 2 added ~24 lines of comment to chat_runner.py, which shifted three of the
five sites the body's Scope table cites. The table said 4170 / 5053 / 5294; the real
lines are 4183 / 5066 / 5307, re-measured on 0bea21daf along with the two
above the change (293, 3203) which did not move. The table is now anchored to
0bea21daf.

Worth naming rather than quietly fixing: the numbers were correct when written and
wrong by the time they were published, because they were measured before the amend
that shifted them. Same defect class as the docstring above -- a claim about the
code that the code moved out from under.

Verification on `0bea21daf`

test/test_eager_spawn_config_load_off_loop.py + test/test_eager_spawn.py --
64 passed.
No read-only claim about the load survives anywhere in the diff except the two
corrected statements that say it is NOT read-only (grepped both changed files).
check_black_formatting.py passes; flake8 and isort clean.
Production change unchanged: still one line. This round touches only the test
docstring.

Author is still @leonlaiyc; both Co-authored-by trailers intact; still one commit.
No /ai-review override used.

chenmingwei23 · 2026-09-02T02:02:44Z

GPT 5.6 blocked the rebased head cc3425394 on a data-loss finding. The mechanism
it names is real in the loader, but it is not reachable at this call site, and I
can show why rather than assert it. Head is now 8161898af, which documents the
invariant that forecloses it.

The finding

BLOCKING -- chat_runner.py:3388 -- Offloaded migration can overwrite a
concurrent config edit. Legacy flat workspace in config.local.json -> eager
worker triggers migration while dashboard PATCH writes config -> stale
cfg.save() lands last and discards the edit. Fix: Revert the offload.

The write-back migration does exist and it does call cfg.save()
(config/loader.py, the needs_migration and not cfg._degraded_sections branch), so
in the abstract a worker-thread migration racing a config write is a genuine hazard.
Two facts remove it from this call site.

Fact 1: the migration is one-shot per process (measured, not read)

Probed against the real loader with a real legacy config -- {"workspaces": {"default": "/tmp/legacy-flat-workspace"}}, exactly the flat-string trigger the
finding names -- with KiroCrewConfig.save instrumented to record every call:

config_path() -> <temp>/config.json
after load #1: 1 save(s) [(<caller thread>, 'save')]
after load #2: 1 save(s) total
after load #3: 1 save(s) total

migration wrote on load #1        : True
migration did NOT write on #2/#3  : True
ONE-SHOT CONFIRMED: only the FIRST load in the process migrates.

Only the first load whose on-disk config still has a legacy shape writes. Every
later load in that process skips the branch entirely.

Fact 2: the offloaded load is never the process's first load on this path

_eager_spawn is created at exactly ONE site -- chat_runner.py:3237, inside
schedule_eager_spawn -- and that function runs its own bare
KiroCrewConfig.load() on the loop at chat_runner.py:3229 first, reaching
create_task only if that load succeeded and session.eager_spawn is true:

try:
    cfg = KiroCrewConfig.load()          # 3229, ON THE LOOP
    if not cfg.session.eager_spawn:
        return None
except Exception:
    return None
...
task = asyncio.create_task(_eager_spawn(state, slot, allow_resume=allow_resume))  # 3237

So a pending migration is always performed by that on-loop load, on the loop,
exactly as before this change. By the time the offloaded load runs, the config is
canonical and needs_migration is False. There is no path where the worker performs
the migration write: it cannot be the first load, because the load that gates its own
creation already ran.

This is the same structural fact the First Principles lane established when it
showed the cold read still lands at 3229 -- it limits what this PR buys, and it
also forecloses this finding.

On the prescribed remedy

"Revert the offload" would delete the fix for #4117 while leaving both real items
untouched: the off-loop load that genuinely can migrate on a worker (the stop-hook
nudge-cap site, already on main and not introduced here) and the cold on-loop read
at 3229. So the remedy does not act on the hazard it names.

What changed instead

Rather than only rebutting, 8161898af documents the invariant at the call site,
because a reviewer reading the offload as "moves a config WRITE to a worker" is a
reasonable misreading that has now cost a round and would recur:

The write-back MIGRATION cannot fire here [...] It is one-shot per process: only
the first load whose on-disk config still has a legacy shape calls save(), and
every later load skips it. This task is created at exactly one site --
schedule_eager_spawn -- which runs its own KiroCrewConfig.load() ON THE LOOP
before it creates the task [...] Deleting that on-loop load (see #7734) must keep
this invariant in view: it would make THIS the process's first load and hand the
migration write to a worker.

That last sentence matters beyond this round: it is a real constraint on #7734. The
subtraction First Principles wants -- delete the 3229 load and gate from the
offloaded cfg -- would make this call the process's first load and thereby create
exactly the race GPT describes. #7734 now has to carry a migration-ordering argument,
which it did not before. Two lanes pointing at the same line from opposite directions
is what surfaced that.

Verification on `8161898af`

test_eager_spawn_config_load_off_loop.py + test_eager_spawn.py -- 64 passed.
check_black_formatting.py, check_sync_io_in_async.py pass; flake8 clean.
Body citations re-measured again for this head (4224 / 5150 / 5391 shifted; 295 and
3229 unchanged).

Production change is still one line. Author is still @leonlaiyc, both
Co-authored-by trailers intact, one commit. No /ai-review override used -- it is
inert on a fork PR in any case.

chenmingwei23 · 2026-09-02T02:51:00Z

GPT 5.6 re-blocked 8161898af with the same finding it raised on cc3425394. Two
independent samples landing the same finding means it is stable, not
non-deterministic noise, so I am stopping rather than re-rolling a third time, and
escalating with what I can and cannot prove.

What is proven safe

The ordinary path cannot reach the migration write, and this is measured rather than
argued:

The write-back migration is one-shot per process. Probed against the real
loader with a real legacy flat-workspace config and KiroCrewConfig.save
instrumented: load test: validate CI workflows on KiroCrew #1 writes once, loads refactor: rename project KiroClaw to KiroCrew #2 and refactor: remove dead legacy compatibility shims #3 write zero times.
The offloaded load is never the process's first load on this path.
_eager_spawn is created at exactly one site, chat_runner.py:3237, inside
schedule_eager_spawn, which runs its own bare KiroCrewConfig.load() on the
loop at 3229 and reaches create_task only if that load succeeded. Any pending
migration is therefore performed by that on-loop load, on the loop, exactly as
before this change.

What I cannot rule out, stated plainly

My rebuttal covers "migration already done before the task existed". It does not
cover the window GPT's second phrasing names: if config.json or
config.local.json acquires a legacy shape during the debounce -- after the 3229
load, before the offloaded one -- then the offloaded load does re-trigger the
migration, and that save() runs on a worker thread with no ordering against a
concurrent config write.

That requires a legacy-format write landing inside a ~debounce-long window,
concurrently with a config update whose loss would matter. I consider it a narrow
edge, but I cannot call it impossible, and the anchor is data loss, so I am not going
to wave it through on my own judgement.

Two further facts the maintainer should weigh, one each way:

Against the finding being new: main already runs
await asyncio.to_thread(KiroCrewConfig.load) at the stop-hook nudge-cap site, so a
worker-thread migration write already exists on main. This PR does not invent the
class.
For the finding mattering more here: eager spawn fires on slot create, project
set, agent switch and slot focus, so this call site is triggered far more often than
the stop-hook one. A pre-existing rare race becoming a frequent one is a real
argument, and I do not want to hide it behind "pre-existing".

Why I did not fix it forward

There is no read path that skips the migration. KiroCrewConfig.load takes no
parameters and the loader has no migrate=False, no read-only variant, and no
peek-style accessor. So every available fix is a change to a shared loader API or to
the loader's write ordering -- not something to fold into an adoption rebase of a
contributor's one-line change, and not something I should decide unilaterally.

/ai-review override is not an option either: it is a same-repo mechanism and is
inert on a fork PR's Stage-2 lanes.

Options

Merge as-is and track the loader-level ordering separately. The ordinary path
is proven safe and the invariant is now documented at the call site; the residual
window is the narrow one above and predates this PR in kind.
Add a no-migrate read path to the loader (load(migrate=False) or a
speculative-read accessor) and use it here. This is the clean fix for a speculative
read that has no business writing anything, but it is a shared-API change and wants
its own PR.
Serialize the write-back migration in the loader -- ticket-order or lock the
migration save() the way publish_autocompact_pct is already ticket-ordered.
This fixes the class for every off-loop load including the one already on main.
Own PR.
Revert the offload, as GPT prescribes, leaving Speculative eager spawn stats, reads and jsonschema-validates config.json on the gateway event loop #4117 unfixed and the cold
on-loop read at 3229 in place.

My read: 1 now with 3 as the follow-up, because 3 fixes the class rather than this one
call site and main already carries the same hazard. But this is a data-loss anchor
on someone else's PR, so it is the maintainer's call, not mine.

State of the PR at `8161898af`

Everything except the GPT lane is green: all Linux and Windows backend shards,
backend lint, frontend, E2E, desktop build, and the other four review lanes on the
previous head (Design PASS, Opus no findings, UX skipped-no-UI, First Principles
advisory CONCERNS, both halves dispositioned). Targeted tests 64/64 locally; black,
sync-io-in-async, flake8 and isort clean.

Author is still @leonlaiyc, both Co-authored-by trailers intact, one commit, two
files, production change still one line. Zero overrides used across all five rounds.

Related: #7734 (the 3229 on-loop load). Note the constraint added in round 5 --
deleting that load would make this call the process's FIRST load and create exactly
the race above, so #7734 now has to carry a migration-ordering argument.

chenmingwei23 · 2026-09-02T02:56:49Z

Final disposition. This PR is handed to a maintainer for the merge decision, with a
complete record. Head 8161898af.

Lane state on this head

lane	verdict
Design Review	PASS -- sound design
First Principles Review	PASS -- justified and minimal (was CONCERNS; upgraded on this head)
Opus 4.8 Review	no blocking findings
UX Review	skipped -- no UI surface
GPT 5.6 Review	blocking -- see below

Every non-review check is green: all Linux and Windows backend shards, backend lint
and type check, frontend lint and tests, E2E, desktop build, CloudFormation lint,
Semgrep, Inclusive Language, PR Hygiene, PR Scope, Fork PR Description, Screenshot
Evidence, Dependency License Gate, Cross-Platform Portability.

The one blocking finding: real, narrow, and not fixable in this PR's scope

GPT 5.6 blocked twice, on cc3425394 and again on 8161898af -- two independent
samples of a non-deterministic lane landing the same finding, so it is a stable
verdict rather than lane noise, and I stopped re-rolling.

What is proven safe. The ordinary path cannot reach the migration write:

The write-back migration is one-shot per process. Probed against the real loader
with a real legacy flat-workspace config and KiroCrewConfig.save instrumented:
load test: validate CI workflows on KiroCrew #1 writes once, loads refactor: rename project KiroClaw to KiroCrew #2 and refactor: remove dead legacy compatibility shims #3 write zero times.
The offloaded load is never the process's first load on this path.
_eager_spawn is created at exactly one site, inside schedule_eager_spawn, which
runs its own bare KiroCrewConfig.load() on the loop first and reaches
create_task only if that load succeeded. Any pending migration is performed by
that on-loop load, on the loop, exactly as before this change.

What I could not rule out. GPT's second phrasing names a window the above does not
close: a legacy shape landing during the debounce, after the on-loop load and before
the offloaded one, would re-trigger the migration on a worker with no ordering against
a concurrent config write. Narrow, but not impossible, and the anchor is data loss.

Why it was not fixed forward here. There is no read path that skips the migration.
KiroCrewConfig.load takes no parameters; the loader has no migrate=False, no
read-only variant and no peek accessor. Every available remedy is a change to a
shared loader API or to the loader's write ordering, which does not belong in an
adoption rebase of a contributor's one-line change.

The class is now tracked as #7793 -- the migration save() is unordered against
concurrent config writes, while its sibling publish_autocompact_pct in the same
load() is already ticket-ordered for exactly this reason. That issue records the
probe, proposes ticket-ordering or locking the write, and notes both facts that cut
each way: the hazard is pre-existing on main via the stop-hook off-loop load, and
it becomes more frequent as speculative reads move off the loop, which is the right
direction for those reads.

/ai-review override was not used and is not applicable -- it is a same-repo
mechanism and inert on a fork PR's Stage-2 lanes. Zero overrides across all five
rounds.

First Principles' remaining advisory: accepted, deliberately not pushed

Its PASS carries one subtraction, and it is right: the
snapshot-handshake-revalidate arrow diagram appears verbatim in both the call-site
comment and the test module docstring, while that same docstring argues two copies of
an argument "would drift". The rule should apply to itself.

Not acted on here, for one reason: any push re-rolls the blocking GPT lane, and this
PR is under escalation precisely because that lane's finding is stable. Trading a
documentation de-duplication for a fresh non-deterministic roll of a hard gate is the
wrong trade at this point. It is a clean follow-up, or a one-line fixup for whoever
takes the merge.

What a maintainer is deciding

Merge as-is and let Config write-back migration save() is unordered against concurrent config writes #7793 fix the class. The ordinary path is proven safe, the
invariant is documented at the call site, and the residual window pre-dates this PR
in kind.
Hold until Config write-back migration save() is unordered against concurrent config writes #7793 lands, then rebase this on top.
Revert the offload as GPT prescribes, leaving Speculative eager spawn stats, reads and jsonschema-validates config.json on the gateway event loop #4117 unfixed and the cold
on-loop read in place.

My read is 1, with #7793 as the follow-up, because #7793 fixes the class for every
off-loop load including the one already on main rather than this one call site. But
it is a data-loss anchor on a contributor's PR, so the call is not mine.

Provenance

@leonlaiyc is the commit author, unchanged through all five rounds. Both
Co-authored-by trailers intact (Claude Opus 5, and the adopting operator). One
commit, two files, production change still one line. This PR was adopted only to
rebase it off a stale base and drive it to green; the change itself is the author's.

Round history, all dispositioned in-thread: the false "load is read-only" claim
(GPT, round 1 -- corrected in comment, commit message and body), the overstated
cold-path harm and incomplete sibling inventory (First Principles, round 1 -- body
corrected, subtraction deferred to #7734), the same false claim surviving in the test
docstring (First Principles, round 2 -- fixed), a main-owned Slack log-site census
breach (cleared by rebasing onto 74ffc2890), and this migration finding (rounds 4-5).
Body file:line citations were re-measured on every head.

bolichen97 · 2026-09-04T06:39:36Z

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 #7161. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4118: KEEP. Independent goals in adjacent code; neither subsumes the other and no conflict is expected. Files: src/kiro_crew/dashboard/chat_runner.py.
This PR is OVERLAPPING with PR #7937. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4118: KEEP. PR #7937 removes the only blocking review finding's mechanism but implements none of PR #4118's behavior: the eager-spawn load is still on the loop in current main. Files: src/kiro_crew/config/loader.py.

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

_eager_spawn resolves the slot's agent bindings before the speculative handshake, and that read is a synchronous KiroCrewConfig.load() on the gateway event loop: path resolution, exists()/read_text()/json.loads() on config.json, is_file()/stat() and a deep merge of any config.local.json overlay, then the full jsonschema validation. None of it yields. The loader's fingerprint cache short-circuits a warm call, so the blocking read is the cold path -- a freshly started gateway, or the first eager spawn after either config file is written. This task is speculative: it exists only to make the slot's first real message faster, nothing depends on its result, and it is already debounced and semaphore-capped. So an optional latency optimisation was doing cold filesystem and schema work on the one loop every live session, WebSocket broadcast and HTTP handler shares. Only the load crosses to a worker. Bindings are resolved, the session acquired and the slot read back on the loop, so no slot or session state is handed to another thread. The new suspension widens an existing window rather than opening a new one. It lands inside the envelope _eager_spawn already maintains: _bound is snapshotted immediately above the load, and the post-handshake guards already discard a session whose creator lost the race, whose slot was deleted or replaced, or whose bindings moved. So a switch or a delete landing during the load is revalidated there, and no new lifecycle state, generation counter or lock is introduced. A worker abandoned by a cancelled eager task is acceptable for a narrower reason than "read-only", which the load is not: it publishes three process-global snapshots and its write-back migration can rewrite config.json. The autocompact publish is ordered on a ticket drawn before the read, so an earlier-starting load cannot overwrite a newer one, and the other two are idempotent rebinds of what was just read. The residue -- those two carry no ordering, so a late worker can republish a superseded value -- is a property of concurrent loads generally, not of this call site; the stop-hook nudge-cap load already runs off-loop. Tests: a thread-identity regression test that fails deterministically on an unfixed tree, plus two preservation tests that hold the loader suspended and fire a slot replacement and an agent switch inside the new window. Refs kirodotdev#4117 Rebased onto current main (the base this branch carried predated the black-baseline gate); the new test file is reformatted with the pinned black==26.3.1 to satisfy it. Production change is byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: gh-autofix#2887 <chenmingwei23@users.noreply.github.com>

bolichen97 · 2026-09-08T19:20:54Z

Rebased onto main 6ae74179d by a maintainer as part of the 2026-09-08 open-PR audit. Branch was 797 commits behind and mergeable_state: dirty.

Conflicts (1 file):

src/kiro_crew/dashboard/chat_runner.py — main's fix(chat): honour global agent.model default for auto-created slots #8834 added loaded_cfg = cfg immediately after the synchronous cfg = KiroCrewConfig.load() in _eager_spawn. Resolved by keeping this PR's cfg = await asyncio.to_thread(KiroCrewConfig.load) (with its comment block) and re-applying main's loaded_cfg = cfg right after it, so _default_session_model still receives the loaded config.

Gates run locally on changed files: black, isort, flake8 all clean; test/test_eager_spawn_config_load_off_loop.py 3 passed; test/test_eager_spawn.py 64 passed.

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

leonlaiyc requested a review from a team as a code owner August 17, 2026 12:38

leonlaiyc force-pushed the fix/eager-spawn-config-load-offloop branch from 5feabb2 to c58fe84 Compare August 18, 2026 06:19

leonlaiyc mentioned this pull request Aug 18, 2026

Speculative eager spawn stats, reads and jsonschema-validates config.json on the gateway event loop #4117

Open

This was referenced Aug 21, 2026

session-tool-policy reads the agent config on the gateway event loop #4945

Closed

fix(dashboard): read the agent tool policy off the event loop #4946

Merged

bolichen97 enabled auto-merge (squash) August 24, 2026 06:59

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026

chenmingwei23 force-pushed the fix/eager-spawn-config-load-offloop branch from 0bea21d to cc34253 Compare September 2, 2026 00:55

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026

chenmingwei23 force-pushed the fix/eager-spawn-config-load-offloop branch from cc34253 to 8161898 Compare September 2, 2026 02:01

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026

chenmingwei23 mentioned this pull request Sep 2, 2026

Config write-back migration save() is unordered against concurrent config writes #7793

Closed

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026

chenmingwei23 mentioned this pull request Sep 2, 2026

fix: order the config write-back migration against concurrent writes #7937

Merged

bolichen97 mentioned this pull request Sep 4, 2026

fix(session): refuse stale sessions at claim time and surface refused clears #7161

Open

javenciu mentioned this pull request Sep 5, 2026

fix: count fresh eager-spawned sessions against the live-population cap #8835

Merged

5 tasks

github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 6, 2026

bolichen97 force-pushed the fix/eager-spawn-config-load-offloop branch from 8161898 to 4a63f79 Compare September 8, 2026 19:20

Navigation Menu

Uh oh!

fix(dashboard): offload eager spawn's agent-binding config load - #4118

fix(dashboard): offload eager spawn's agent-binding config load#4118
leonlaiyc wants to merge 1 commit into
kirodotdev:mainfrom
leonlaiyc:fix/eager-spawn-config-load-offloop

leonlaiyc commented Aug 17, 2026 •

edited by chenmingwei23

Loading

Uh oh!

bolichen97 commented Aug 21, 2026

Uh oh!

chenmingwei23 commented Sep 1, 2026

Uh oh!

chenmingwei23 commented Sep 2, 2026

Uh oh!

chenmingwei23 commented Sep 2, 2026

Uh oh!

chenmingwei23 commented Sep 2, 2026

Uh oh!

bolichen97 commented Sep 4, 2026

Uh oh!

bolichen97 commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

leonlaiyc commented Aug 17, 2026 • edited by chenmingwei23 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem / Motivation

Why it matters

What changed (motivation → approach → change)

Why this is safe: the suspension lands inside an existing envelope

Tests

Scope

Pattern harvest

Uh oh!

bolichen97 commented Aug 21, 2026

Uh oh!

chenmingwei23 commented Sep 1, 2026

First Principles subtraction 1: the false claim in the test module docstring

First Principles subtraction 2: delete the bare load at schedule_eager_spawn

Line numbers in the PR body were stale, and are corrected

Verification on 0bea21daf

Uh oh!

chenmingwei23 commented Sep 2, 2026

The finding

Fact 1: the migration is one-shot per process (measured, not read)

Fact 2: the offloaded load is never the process's first load on this path

On the prescribed remedy

What changed instead

Verification on 8161898af

Uh oh!

chenmingwei23 commented Sep 2, 2026

What is proven safe

What I cannot rule out, stated plainly

Why I did not fix it forward

Options

State of the PR at 8161898af

Uh oh!

chenmingwei23 commented Sep 2, 2026

Lane state on this head

The one blocking finding: real, narrow, and not fixable in this PR's scope

First Principles' remaining advisory: accepted, deliberately not pushed

What a maintainer is deciding

Provenance

Uh oh!

bolichen97 commented Sep 4, 2026

Open PR relationship audit

Relationship findings

Uh oh!

bolichen97 commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leonlaiyc commented Aug 17, 2026 •

edited by chenmingwei23

Loading

First Principles subtraction 2: delete the bare load at `schedule_eager_spawn`

Verification on `0bea21daf`

Verification on `8161898af`

State of the PR at `8161898af`