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

[chenmingwei23]

Problem / Motivation

KiroCrewConfig.load()'s write-back migration called cfg.save(), which
re-serializes the whole snapshot that load had already parsed. Nothing ordered
that write against any other config writer, so:

1. worker thread: load() reads config.json, sees a legacy shape, decides to
   migrate
2. event loop: a dashboard PATCH (or kirocrew config set) writes a newer config
3. worker thread: cfg.save() writes the snapshot from step 1

ends with step 2's edit gone. The loss is silent and the lost data is user
configuration.

The migration is one-shot per process, so the window is the first load in a
process whose stored config still has a legacy shape (no agents, a
default_agent not present in agents, or -- where jsonschema is unavailable --
flat workspace strings).

Why it matters

This is pre-existing on main, not introduced by any open PR: chat_runner.py
already runs await asyncio.to_thread(KiroCrewConfig.load) at the stop-hook
nudge-cap site, so a worker-thread migration write is reachable today. It becomes
materially more likely as more latency-sensitive reads move off the loop, which
is the right direction for those reads (#4117 / #4118) -- a fix that is correct
for the event loop should not have to carry an argument about a loader-level
write race. #7734's proposed subtraction would make an offloaded load the
process's first load, and so depends on this.

The tell is a strong one: inside one function, load(), three publishes and one
write-back all mutate shared state, and exactly one of the four --
publish_autocompact_pct -- is ordered, with a docstring that spells out the
hazard ("a load that began earlier cannot overwrite a newer one"). The heaviest
mutation of the four had no equivalent.

What changed (motivation -> approach -> change)

Root cause: a read-modify-write with no precondition. cfg is read at the top of
_load_resolved; save() writes it back at the bottom, unconditionally and in
full. Anything that landed in between is replaced.

The issue proposed two shapes. Both were checked against the code and neither is
sufficient on its own:

• An ordering ticket like publish_autocompact_pct's orders load against
  load. The writer being lost here is not a load -- next_config_load_ticket has
  no caller outside config/loader.py, so a dashboard PATCH never draws a ticket
  and the comparison never sees it.
• A lock around the write does not help either, because the read happened
  before the lock. The read has to move inside it.

So the migration write now has two properties, separated because only one of them
can be applied everywhere:

• The write is a delta. _apply_document_migrations applies only the keys the
  load decided on, to the document as read at write time, re-deciding each one
  against that document. A concurrent write to any other setting is not merely
  ordered but untouchable -- those bytes are never part of our write. Verified: a
  migrating load now rewrites 4 top-level keys instead of materializing every
  section in the schema.
• The read and the write are one critical section, via the repo's
  update_config_locked -- the required path for config.json mutations, which
  re-reads inside an advisory lock and writes through
  write_config_atomically. So the migration's own keys are decided from current
  state and cannot interleave with another locked writer.

The load keeps deciding whether to migrate, and now records which migrations
it decided on (pending), so the write never widens beyond what the merged
base+overlay view asked for. Re-checking each entry at write time also makes the
write idempotent: a migration another writer already performed returns "nothing
to do" and skips the write entirely, rather than rewriting a file we agree with.

One containment detail. update_config_locked takes its lock on a <path>.lock
sidecar, and load() reads whatever config_path() resolves to -- which callers
redirect at their own temp files. A sidecar beside such a path is exactly the
orphan class that produced 72k stray .bak files on one dev host, and
TestMigrationBackupContainment pins that a migrating load() leaves a
caller-owned directory as it found it. So the same containment predicate that
already gated the backup now also gates the lock, extracted as
_inside_data_home: contained (always true in production, where config_path()
is config_dir() / "config.json"), take the lock; redirected, do the delta off an
immediate fresh read and leave no sidecar. What the redirected path gives up is
ordering on the migration's own keys against an unlocked writer of those same
keys -- and a redirected config has no gateway writing it, since the dashboard and
CLI paths write config_path().

The lock is taken WITHOUT waiting, which is the second thing to get right:
load() is reached from the event loop all over the tree, so a POSIX flock wait
here would trade a rare data-loss race for a gateway stall lasting as long as the
holder keeps the lock. Giving up costs nothing, because a held lock means another
writer is mid-write and that writer's bytes are precisely what must not be
clobbered -- declining is the correct outcome, not a compromise, and the migration
is already retry-on-next-load by construction. So platform_compat.file_lock
gains wait=False (POSIX LOCK_NB; Windows already behaves this way on the loop
thread, and a zero timeout makes it uniform) raising BlockingIOError, which
update_config_locked exposes as wait_for_lock. Both default to the existing
waiting behavior, so all 69 existing call sites are unchanged. BlockingIOError
is an OSError, so it narrows rather than widens what a caller must handle, and
it separates "someone is writing right now" from that function's existing
stuck-holder ceiling. The remaining file I/O on the loop -- one read, one atomic
rename -- is what cfg.save() did here before, unchanged.

The backup now happens immediately before the write (inside the lock hold where
there is one), so it captures the bytes actually being replaced rather than
whatever was there when the load started. A failing copy still propagates and
aborts the write, preserving the existing contract: a config we could not copy
aside is not rewritten, and the migration retries on the next load.

One consequence worth stating plainly, because it is visible on disk: a migrating
load no longer materializes every section of the schema into config.json. Only
the keys it migrates are written. That is the delta property, and it is also
strictly better for the separate defect where a materialized key pins a shipped
default forever (#5244) -- fewer keys are pinned. One existing test asserted on
the old side effect rather than on its own subject: test_put_cannot_add_a_gitlab_ host read raw["dashboard"]["gitlab_hosts"] back out of the file, with a comment
explaining the key was present only because "cfg.save() serializes every dataclass
field". Its security property -- a dashboard caller cannot authorize a new GitLab
instance -- is unchanged and still asserted; the raw-file assertion now reads the
stored VALUE (absent or empty are the same thing to the loader, which the sibling
cfg.dashboard.gitlab_hosts == [] assertion proves) instead of requiring the key
to exist.

Deliberately out of scope: the issue's optional load(migrate=False) read-only
path. It is a new public API surface, not part of the ordering defect.

Also out of scope, and worth naming because a reviewer raised it: the migration's
lock does not serialize against the legacy config writers that call
write_config_atomically directly under the in-process asyncio _get_config_lock
(the dashboard agents endpoint, updates.py, security.py, messaging.py,
mcp.py, core.py STT). update_config_locked's own docstring names them as
pending conversion. That gap is shared by every one of its ~69 existing call
sites, it predates this change, and this change strictly narrows it -- before, the
migration took no lock at all and rewrote the whole document; now it holds the
sidecar lock and writes three keys. Closing it properly means converting those
writers, not special-casing this one caller, so it is filed as #8032 with the
sites named.

Accepted residue, recorded so the merge carries it: the seeded agent's kiro agent
is the merged value of agent.default_agent, which comes from two independently
written files. config.local.json is read outside the base lock, so a
config set --local agent.default_agent landing between that read and the base
write leaves the seed one write stale. Closing it needs a two-file critical
section that does not exist today and that no other writer participates in;
#8032 is the path that makes one possible. Seeding empty and resolving live is not
an alternative -- resolve_agent_bindings computes
kiro_agent = passthrough or agent_cfg.kiro_agent with no fallback to
agent.default_agent, so an empty seed yields an empty binding.

Tests

TestMigrationWriteBackOrdering in test/test_config_loader.py:

• test_a_write_landing_mid_migration_survives -- the deterministic two-writer
  interleave. Writer A is a migrating load() suspended after it has decided to
  migrate but before its bytes reach the file; writer B is an ordinary
  update_config_locked write (the shape the dashboard PATCH and kirocrew config set both use) setting session.autocompact_pct. Both must be
  observable afterwards. Threads are started and drained in a try/finally so a
  timeout or assertion cannot leave a writer running against this test's paths.
• test_the_migration_only_rewrites_the_keys_it_owns -- parametrized over the
  data-home path (which also takes the lock) and a redirected path (which cannot,
  without leaving a sidecar). Asserted on the top-level key set, because a
  snapshot rewrite materializes every section in the schema.
• test_a_held_lock_defers_the_migration_instead_of_waiting -- with the lock held
  by another descriptor, the migration returns without writing. Run on a thread
  with a bounded join and asserted inside the hold, so a regression to a waiting
  acquire FAILS on an assertion instead of hanging the suite.
• test_a_migration_another_writer_already_did_writes_nothing -- the re-read
  turns a stale reason to write into a no-op, with no redundant .bak.
• test_the_seeded_agent_takes_its_kiro_agent_from_the_reread -- the seeded
  agent's kiro_agent comes from the document read inside the lock, not from the
  load's older snapshot, so an agent.default_agent change landing in between is
  honored rather than silently reverted for every default session.
• test_the_seed_falls_back_when_the_document_names_no_kiro_agent -- with nothing
  stored, the fallback is the load's resolved value (which carries the overlay and
  the dataclass default) rather than a literal.

RED verified against the base commit with the source reverted and the tests kept:
the interleave test fails on "the config write that landed while the migration was
in flight was lost", and the delta tests on "the migration wrote sections it did
not migrate". Two fixes are mutation-verified individually: flipping
wait_for_lock=False to True reddens the held-lock test on its assertion, and
seeding from the snapshot instead of the re-read reddens the seed test. The
pre-existing TestMigrationBackupContainment guards pass unchanged -- the lock
containment above is there because the first draft broke them.

Run: test_config_loader.py + test_dashboard_config_gitlab_hosts.py 510 passed;
test_platform_compat*.py + test_no_blocking_call_on_loop.py +
test_loop_lock.py 585 passed / 22 skipped; 20 further test_config* /
test_atomic_write* / test_autocompact_default / lock-regression files 586
passed / 1 skipped. black, flake8 and isort clean on all four changed files; mypy
clean on both source files.

Manual verification

N/A -- the defect is a thread interleave with no user-visible surface, and the
interleave is reproduced deterministically in the test above rather than by hand.

Related Issues

Closes #7793

Follow-up filed from this review: #8032 (legacy config writers bypass the advisory
lock), which carries the accepted residue named above.

Pattern harvest

Rule candidate: review-prompt

Pattern: a function that both READS shared state and WRITES it back must carry
the same ordering guard as its siblings, or say in-comment why it does not need
one. The specific tell is checkable by a reviewer rather than a linter: in one
function, several mutations of the same shared state where exactly one is ordered
and its docstring names the hazard -- an unguarded sibling doing a heavier
mutation is a defect, not a deliberate asymmetry.

Second, narrower rule: when the guard you add is a lock, check two things about
it. Where its lockfile lands -- an advisory-lock sidecar is a file, and a file
beside a path the process does not own is litter, the same containment question
the backup in this very function had already been fixed for. And whether the
acquire can WAIT on a thread that must not block: converting an unordered write
into a correctly ordered one is not a win if the ordering is bought with a
blocking wait on the event loop. For work that is optional and retried, the
answer is a single-shot acquire that declines.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 0dc8a392e6b1eab64431a0f04cf96f21612a81e5 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix for a real lost-update race: read moves inside the lock, the write shrinks to a delta, and every failure path defers safely.

Alternatives (ticket ordering, lock-around-write-only) are examined and correctly rejected; both new keywords (wait, wait_for_lock) default to existing behavior so all prior call sites are untouched; the non-blocking acquire fails toward retry-next-load, never toward an unserialized write; the acknowledged residual gap (legacy in-process-lock writers) is pre-existing and strictly narrowed. The one long-term cost — each migration now implemented twice (parsed-view decision, raw-document delta) — fails benign: drift makes the re-check decline the write, never widen it, so it does not rise to a concern for three one-shot legacy migrations. Description ↔ diff fidelity holds, including the gitlab-hosts test change, whose security assertion survives.

[DESIGN-REVIEWED] 0dc8a39

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 0dc8a392e6b1eab64431a0f04cf96f21612a81e5 — 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.

All evidence gathered. The only premise-level finding: the repo already has one non-blocking lock primitive (try_acquire_lock, ~15 consumers), and this PR adds a second spelling via file_lock(wait=False) with exactly one consumer — the two already diverge in error semantics. Everything else in the change is declared, cause-level, and counted. Final review:

First-Principles-Verdict: CONCERNS

file_lock gains a wait= param the repo already provides as try_acquire_lock — a second non-blocking-acquire spelling with exactly one consumer.

What this change ships

Intent: stop a first-load write-back migration from silently overwriting a config change saved while it ran — a FIX.

1. A settings write landing mid-migration now survives — justified, cause-level (read moved inside the lock)
2. Migrating load writes only migrated keys, no full-schema materialization — justified, declared
3. Contended lock defers migration to next load with an info log — justified (event-loop stall)
4. Already-migrated file → no write, no .bak — justified
5. Backup captures bytes at write time, not load time — justified
6. Symlinked-out config gets no .lock sidecar beside its target — justified (documented 72k-orphan incident)
7. update_config_locked(wait_for_lock=) — one consumer (loader.py:768), minimal plumbing
8. platform_compat.file_lock(wait=) — one consumer, duplicate of try_acquire_lock (platform_compat.py:666)
9. Loader-internal migration helpers and MIGRATE_* names — internal, justified
10. gitlab-hosts test asserts stored value, not key presence — declared, property preserved

Watch

• session_ledger.py:187 documents try_acquire_lock as "the repo's one non-blocking acquire primitive, covering POSIX and Windows alike". wait=False makes that two, with one consumer (loader.py:1219; grepped file_lock\([^)]*wait), and they already diverge: try_acquire_lock swallows any OSError as "not taken" while wait=False raises BlockingIOError for contention only. Both spellings must now be maintained.
• The unfixed siblings (legacy writers on _get_config_lock alone) are counted and named in update_config_locked's own docstring (loader.py:1126-1128) — accepted-and-deferred, not a demand.

Subtractions

• Drop wait= from platform_compat.file_lock (1 consumer): in update_config_locked's wait_for_lock=False branch, use the existing try_acquire_lock(fd, exclusive=True) + release_lock pair and raise BlockingIOError on refusal — same behavior, zero new surface on a primitive with ~69 call sites.

[FIRST-PRINCIPLES-REVIEWED] 0dc8a39

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 0dc8a392e6b1eab64431a0f04cf96f21612a81e5 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 0dc8a39

Verdict parsed from the review's SHA-scoped output markers for commit 0dc8a392e6b1eab64431a0f04cf96f21612a81e5.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 0dc8a392e6b1eab64431a0f04cf96f21612a81e5: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 0dc8a392e6b1eab64431a0f04cf96f21612a81e5 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0dc8a39

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 0dc8a392e6b1eab64431a0f04cf96f21612a81e5: <one-sentence reason>

[chenmingwei23]

Both GPT 5.6 findings on b6caed4a3 were real. Fixed in 0f73920b5.

BLOCKING 1 -- migration can block the gateway event loop. Correct, and the
more important of the two: load() is reached from the event loop all over the
tree, and POSIX file_lock blocks in fcntl.flock indefinitely, so the first
draft traded a rare data-loss race for a stall lasting as long as the holder kept
the lock. That is a worse bug than the one being fixed.

Taken the finding, not the prescribed fix. Skipping persistence when
on_event_loop() is true would mean a gateway that only ever loads config on the
loop never migrates at all, and it adds a loop/thread asymmetry to a path that has
none. Instead the acquire never waits, on either thread: file_lock gains
wait=False (POSIX LOCK_NB; the Windows branch already behaves this way on the
loop thread, so a zero timeout just makes it uniform) raising BlockingIOError,
which update_config_locked exposes as wait_for_lock. Both default to today's
waiting behavior, so all 69 existing call sites are byte-identical.

Declining costs nothing, which is why not waiting is the right answer rather than
a compromise: a held lock means another writer is mid-write, and that writer's
bytes are precisely what must not be clobbered. The migration is already
retry-on-next-load by construction -- the sibling degraded-sections branch relies
on the same property -- so the next uncontended load performs it.
BlockingIOError is an OSError, so it narrows rather than widens what callers
must handle, and it separates "someone is writing right now" from that function's
existing stuck-holder ceiling. The remaining loop I/O -- one read, one atomic
rename -- is what cfg.save() did here before, unchanged.

Pinned by test_a_held_lock_defers_the_migration_instead_of_waiting, and
mutation-verified: flipping wait_for_lock=False back to True reddens it. The
test runs the migration on a thread with a bounded join and asserts inside the
hold, so the regression fails on an assertion instead of hanging the suite -- a
waiting acquire would park until the holder releases, which never happens from
that test's point of view.

BLOCKING 2 -- failure paths leak daemon writer threads. Correct as stated.
Both threads are now created before the try, started inside it, and the
finally sets the release event and drains whichever is still alive before the
patches come off -- so an assertion or a timeout cannot leave a writer running
against that test's paths and patched state.

One note on the first fix's blast radius, since it touches a shared primitive:
platform_compat.file_lock and update_config_locked both gained a
keyword-only parameter defaulting to current behavior. The default call into
_win_acquire_blocking is deliberately still made with no keyword, so the
existing stub in test_platform_compat_coverage.py keeps working -- an earlier
draft passed timeout= unconditionally and broke it.

Verification on 0f73920b5: test_config_loader.py +
test_platform_compat*.py + test_no_blocking_call_on_loop.py +
test_loop_lock.py 1087 passed / 22 skipped; 20 further test_config* /
test_atomic_write* / lock-regression files 586 passed / 1 skipped. black,
flake8, isort clean on all three changed files; mypy clean on the two source
files.

[bolichen97]

Audit note — #7830 is being closed in favour of this PR

Two independent adjudicators both ruled this the surviving implementation.

What the two shared

Same anchor: origin/main src/kiro_crew/config/loader.py:3263-3264 is _write_migration_backup(path) + cfg.save(); #7830's only source hunk replaces line 3264 with if not cfg.save(ticket=ticket):, #7937's @@ -3257,14 +3461,19 @@ deletes both and calls _persist_config_migration(...). Both bodies end "Closes #7793". Survivor proof: git grep next_config_load_ticket origin/main -- src/ test/ returns only config/loader.py and test/test_autocompact_default.py, and #7830 assigns _CONFIG_WRITE_TICKET only inside KiroCrewConfig.save() — but dashboard/handlers/core.py:2129 (await asyncio.to_thread(update_config_locked, cfg_path, mutate=_mutate_config_patch)) and cli_config.py:201 (update_config_locked(config_path(), mutate=_mutate_base)) never draw a ticket, so #7830's comparison cannot see the dashboard PATCH or kirocrew config set it claims to protect. Duplicated coverage: TestMigrationSaveOrderingAgainstConcurrentWrites (#7830, +180 lines) vs TestMigrationWriteBackOrdering (#7937, +255 lines) in test/test_config_loader.py, both patching loader_module._write_migration_backup to land a concurrent write inside the race window.

Why this one

Yes — #7937 is correct, and it is the survivor on code facts rather than on the nomination. #7830 does not close the case it names: its ticket high-water is bumped only in KiroCrewConfig.save(), so a dashboard PATCH or kirocrew config set (both via update_config_locked, neither drawing a ticket) is still overwritten by the migration's full stale snapshot, and the counter is process-local so a cross-process CLI write is unordered too. #7937's delta-plus-advisory-lock write is the shape main's own docstring already calls the required path for config.json mutations, and it additionally makes every non-migrated key untouchable regardless of writer. #7937's two open GPT blocking findings (the lock-less cfg.save() residual, and default_kiro_agent=cfg.agent.default_agent being seeded from the stale snapshot at loader.py:3472) are quality work inside its own design, not a reason to prefer #7830 — the second is a one-line move of the seed into _apply_document_migrations, and #7830 has no answer to either.

What #7830 had that this PR does not

Please pick these up (or say they are not wanted):

One item, and it is a test shape plus a named residual, not the mechanism. #7830's test_concurrent_write_between_read_and_migration_save_survives uses a concurrent writer that is KiroCrewConfig.load() + mutate + cfg.save() — a writer class that takes no <path>.lock sidecar (dashboard/handlers/agents.py:2709/3024/3125/3161, handlers/files.py:1598/1670/1700, handlers/updates.py:1761, cli_commands.py x6, cli_config.py:221). #7937's locked delta therefore still has a read-to-rename window against those writers for the migration's own agents/default_agent keys, which is exactly GPT 5.6's still-open BLOCKING #1 on #7937 head 0f73920b5. Carry that writer shape into #7937 as a follow-up test and name the class in its deferred list (open PR #7167's run_config_write helper is the pre-existing route for it). Do NOT carry save(ticket=...), _CONFIG_WRITE_TICKET or _CONFIG_WRITE_TICKET_LOCK: under #7937 the migration no longer calls save(), so the parameter would land with no caller, and being an in-process module global it orders nothing against a separate CLI process.

---

_{From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.}

[chenmingwei23]

Round 2 on ce246026d (rebased onto current main, previously 0f73920b5). One GPT
finding accepted, one rebutted with evidence, plus a CI regression of mine that was
not in any review comment.

BLOCKING 2 -- migration seeds from a stale default agent. Correct, and it was a
real defect in this PR's own logic. The whole point of the change is that the
migration re-decides against the document read inside the lock, and the seed was
the one value still coming from the pre-lock snapshot -- so an
agent.default_agent change landing in between was silently reverted, and every
default Crew session would have dispatched the superseded agent.

Fixed exactly as prescribed: _apply_document_migrations now reads
agent.default_agent from the re-read document. The caller's value is kept as the
FALLBACK rather than dropped, because it carries the config.local.json overlay
and the dataclass default, which a raw document read cannot see. Two tests pin
both halves, and the document-wins half is mutation-verified: seeding from the
snapshot again reddens test_the_seeded_agent_takes_its_kiro_agent_from_the_reread
on its message.

BLOCKING 1 -- migration still races legacy dashboard writers. The mechanism is
real and I verified it rather than taking it on faith: _get_config_lock() in
dashboard/handlers/agents.py is a LoopBoundLock used with async with, and
that module writes through write_config_atomically directly at three sites (750,
945, 1267), so it never takes the <path>.lock sidecar. A write from there can
still be lost.

I am not applying the prescribed fix, for three reasons.

It is not introduced here, and this change narrows it. Before this PR the
migration took NO lock at all and re-serialized the WHOLE document, so any
concurrent write lost everything. Now it holds the sidecar lock and writes at most
three keys, each re-decided against the document. On the specific scenario in the
finding, agents is only seeded when the re-read document still has none -- so an
agent created before the locked read now survives, where previously it did not.

It is class-wide, not specific to this caller. Every one of update_config_locked's
~69 existing call sites has the identical exposure to those same unlocked writers,
and the function's own docstring names them as "legacy writers that pre-date this
function ... pending conversion". Special-casing the migration would imply the
other call sites are wrong, and would not fix the class. The First Principles lane
reached the same disposition independently on the previous head, recording the
remaining whole-snapshot cfg.save() sites as accepted-and-deferred rather than a
demand on this PR.

And the prescribed remedy is a revert. "Skip write-back until it can also
serialize with legacy _get_config_lock() writers" means the migration never runs:
config.json never normalizes and no .bak is ever written. Bridging to that lock
is also not available to this code path -- it is an asyncio lock and
_persist_config_migration is synchronous and may run on the loop thread, so
acquiring it would either be impossible or reintroduce round 1's loop-blocking
finding in a new form. The real fix is converting those writers onto
update_config_locked, which is a separate change across six-plus endpoints.

A regression of my own, found by CI rather than by review.
test_dashboard_config_gitlab_hosts.py::test_put_cannot_add_a_gitlab_host failed
on both Backend Tests (3.12, 2) and Backend Tests (Windows) (2) with
KeyError: 'gitlab_hosts'. A/B against the base commit confirmed it as mine, not a
flake: green with the two source files reverted, red with them.

Cause: the test read raw["dashboard"]["gitlab_hosts"] back out of the file, and
its own comment said the key was there only because "cfg.save() serializes every
dataclass field". A migrating load no longer materializes unrelated sections -- that
is the delta property this PR ships. The test's subject, that a dashboard caller
cannot authorize a new GitLab instance, is untouched and still asserted; only the
assertion that depended on the side effect changed, and it now reads the stored
VALUE (absent and empty are the same thing to the loader, which the sibling
cfg.dashboard.gitlab_hosts == [] assertion proves) rather than requiring the key
to exist. Worth flagging explicitly since this is an edit to a security test: the
security property is unchanged, and the PR body states the materialization change
as intended behavior.

Still open, and not yet attributable. Backend Tests (Windows) (4) failed on
test_webhooks_api.py::TestOneTurnPerSessionKey::test_concurrent_same_key_is_claimed_before_capacity_await
with a TimeoutError on a 1-second asyncio.wait_for. It passes locally (4/4),
api_hooks_agent has no config load on its path, and the file_lock change leaves
the Windows default acquire byte-identical (_win_acquire_blocking(fd) with no
keyword, deliberately, so the existing stub in test_platform_compat_coverage.py
keeps working). Three unrelated open PRs have all four Windows shards green, so I
am not claiming it is main-owned either. This push re-runs it; if it reproduces on
ce246026d I will treat it as mine and dig rather than call it a flake.

Coverage Gate was a downstream aggregate of the two shard failures.
Dependency Audit fails on four high fast-uri advisories in
website/electron/package-lock.json, against a diff with zero lockfile or
frontend lines.

Verification on ce246026d: test_config_loader.py +
test_dashboard_config_gitlab_hosts.py 510 passed; test_platform_compat*.py +
test_no_blocking_call_on_loop.py + test_loop_lock.py 585 passed / 22 skipped.
black, flake8, isort clean on all four changed files.

[chenmingwei23]

Round 3 on ed43b48a1 (was ce246026d). Accepted, but synthesized rather than
applied as prescribed, because this finding and round 2's are the same property
seen from opposite sides.

Round 2 said the seed must come from the freshly re-read base document, because a
config set agent.default_agent landing after the load's read was being reverted.
Round 3 says the base document must NOT decide it, because config.local.json
wins the deep-merge and an overlay-selected agent was being replaced. Both are
correct about their own hazard, and taking either single source is wrong in the
other's direction -- so applying round 3 by reverting to the snapshot would just
re-break round 2 on the next roll.

The fixed point is that the seed has to be the MERGED effective value -- that is
what the default crew dispatched before the migration existed -- computed against a
CURRENT base. So the seed now resolves the same three-way precedence the loader
itself applies:

1. config.local.json's agent.default_agent, read fresh via the new
   _overlay_kiro_agent(). Where the overlay names the field, the base cannot be
   the effective value.
2. otherwise the base document's own agent.default_agent, as read inside the
   lock -- so a concurrent config set is honored (round 2).
3. otherwise the value the load resolved, the only one carrying the dataclass
   default.

The overlay is read OUTSIDE the lock deliberately: it is user-owned and never
written back, so there is no update of ours to lose against it, and a concurrent
edit to it is the operator's own action rather than a race. It is also read
best-effort -- an unreadable or malformed overlay means "the overlay says nothing
here" and the base value stands, matching the loader, which warns and marks such a
file degraded rather than refusing to load. A stricter rule here would let one
broken user-owned file block a write that is correct without it.

Worth stating for the record on the prescribed alternative ("or revert this
migration rewrite"): reverting the seed to the load's snapshot is what round 2
already rejected, and reverting the migration rewrite wholesale removes the fix
this PR exists for.

Two tests pin the new precedence, and the overlay-first half is
mutation-verified -- dropping overlay_kiro_agent from the chain reddens
test_the_overlay_wins_the_seed_over_the_base_document on its message:

• test_the_overlay_wins_the_seed_over_the_base_document -- base says one agent,
  overlay another; the seed takes the overlay's, and the overlay file itself comes
  back byte-identical.
• test_a_malformed_overlay_lets_the_base_value_stand -- unparseable overlay,
  base value used, write still happens.

Also confirmed on ce246026d, closing both items I left open last round: all four
Backend Tests (Windows) shards passed, so the shard-4 webhooks TimeoutError was
a runner flake and not this change; and Dependency Audit now has a SUCCESS entry,
so the fast-uri advisories were fixed on main during the rebase window and that
red is gone rather than merely attributed.

test_config_loader.py 506 passed on ed43b48a1; black, flake8, isort clean.

[chenmingwei23]

Round 4 on 0dc8a392e (was ed43b48a1). The non-blocking FINDING is fixed. The
BLOCKING one is real and I am NOT patching it -- I am escalating it, because this is
the fourth consecutive round on the same two lines and the recurrence is a
structural conflict rather than a sequence of mistakes.

FINDING (loader.py:495) -- symlink containment. Fixed. Correct, and it was my
own inconsistency: I reused a predicate written for the backup, which writes
<path>.bak beside the path AS GIVEN, to decide the lock, which
update_config_locked places beside the RESOLVED target. A config.json inside the
data home symlinking out of it was therefore classified contained and dropped its
.lock in someone else's directory -- the same orphan class the backup gate exists
for, one indirection further out. The lock decision now asks about the path the
sidecar will actually land beside, via a new _lock_target(). Pinned by
test_a_symlink_out_of_the_data_home_gets_no_lock_sidecar and mutation-verified:
reverting to the unresolved path reddens it on the orphan it names.

BLOCKING (loader.py:724) -- concurrent local override. Real; escalating.

The mechanism is right, and the prescribed fix is implementable -- I checked rather
than assuming. config set --local does go through
update_config_locked(config_local_path(), ...), so it holds a
config.local.json.lock sidecar that could be held here too, and since no writer in
the tree holds BOTH sidecars, taking them in a fixed order could not deadlock
against any of them.

I am escalating instead of applying it, on three grounds.

This is round 4 on the same two lines, and each round found a defect in the
previous round's fix.

round	shipped	next round found
2	seed from the load's snapshot	snapshot is stale vs a concurrent base write
2 fix	seed from the base doc re-read inside the lock	base ignores the overlay, which wins the merge
3 fix	consult the overlay first, then the base	the overlay read is outside the lock
4 (this)	?	the four dashboard overlay writers do not take that lock either

Every one of those fixes was correct about its own hazard. That pattern is the
signature of two requirements that cannot both be satisfied directly, not of
carelessness, and the honest move is to name the conflict.

The conflict. The seeded agents.default.kiro_agent must equal the MERGED
effective value of agent.default_agent at migration time -- that is what the
default crew dispatched before the migration existed, so anything else silently
rebinds it. That value is derived from TWO independently written files, and the repo
has no primitive for an atomic read across both. Any fix that reads them separately
has a window; closing it requires a two-file critical section that does not exist
today and that no other writer participates in.

I checked the one alternative that would dissolve rounds 2-4 together -- seed
kiro_agent="" and let it be resolved live on every load, so there is nothing to
snapshot and nothing to race. It does not work: resolve_agent_bindings computes
kiro_agent = passthrough or agent_cfg.kiro_agent with no fallback to
agent.default_agent, so an empty seed yields an empty binding rather than
inheriting. That is why the seed genuinely has to copy a cross-file value.

Why I am not shipping the two-lock version unilaterally. It would have this
migration hold two nested advisory locks in an ordering rule that exists nowhere
else in the tree, invented by this PR, in a shared loader, on a path reached from
the event loop -- and it would create a config.local.json.lock sidecar even where
no overlay file exists, since serializing against a writer that creates the overlay
requires creating its lockfile. All of that for a window of one stamp + atomic rename on a path that runs at most once per process. And it would still not cover
the overlay writers in dashboard/handlers/mcp.py, security.py, telemetry.py
and apps/manager.py, which do not appear to take that lock -- so the predictable
round 5 is the same finding again with those named instead.

What the residual actually is, stated plainly. On the first load of a
legacy-shaped config in a process, a config set --local agent.default_agent
landing between the overlay read and the base write leaves the seeded crew pointing
at the superseded agent, durably. It is strictly narrower than the window this PR
closes, and narrower than the one the issue reported.

Recommendation, for whoever decides. Merge as is and fix the class properly by
converting the remaining direct write_config_atomically config writers onto
update_config_locked -- which its own docstring already lists as pending -- rather
than growing a bespoke two-lock protocol in the loader. If a maintainer would rather
have the two-lock version in this PR, say so and I will implement it in the next
push; I have not done it unilaterally because it adds a locking discipline to shared
code and that is an owner's call, not a reviewer-round reflex.

test_config_loader.py 507 passed on 0dc8a392e; black and flake8 clean.

[chenmingwei23]

Round 5 on 0dc8a392e. This one I am rebutting on evidence, and I am asking a
maintainer to adjudicate the PR, because five rounds have now landed on the same
function and two of the demand pairs are mutually inconsistent.

BLOCKING (loader.py:768) -- "migration performs synchronous file I/O on the event
loop". Not introduced here; main does the same I/O on the same line.

The migration on main is:

_write_migration_backup(path)   # shutil.copy2 -- a full file COPY
cfg.save()                      # to_dict() over ~40 dataclasses,
                                # config.local.json.is_file() + read_text(),
                                # _subtract_overlay, write_config_atomically

So main already performs, on whatever thread load() is called from: a file
copy, a stat and read of config.local.json, and an atomic write (mkstemp + write

• rename). This PR replaces cfg.save() with one read_config_for_update() plus a
  delta plus the same write_config_atomization. It ADDS one read of config.json
  and REMOVES the whole-document serialization and the subtraction read. The set of
  blocking syscalls on the loop is comparable, and on the write side smaller.

If synchronous migration I/O on the loop is unacceptable, that is a pre-existing
defect in load() worth its own issue -- not something this PR introduced, and not
something it can fix without changing what load() is.

Round 1 raised the loop concern in its accurate form: a POSIX flock WAIT, which
is unbounded and genuinely new, because save() took no lock. I accepted that and
fixed it -- file_lock(wait=False) / update_config_locked(wait_for_lock=False),
single-shot, deferring on contention, mutation-verified by
test_a_held_lock_defers_the_migration_instead_of_waiting. Round 5 has moved from
the unbounded wait to the file I/O itself, and the prescribed remedy is the same one
round 1 proposed and I declined with reasons that still hold: skipping when
on_event_loop() is true means a gateway that only ever loads config on the loop
never migrates at all, and it introduces a loop/thread asymmetry on a path that has
none.

Asking for adjudication, because the rounds are not converging.

round	finding	disposition
1	blocking flock wait on the loop	ACCEPTED -- non-waiting acquire
1	test leaked daemon threads	ACCEPTED -- try/finally drain
2	seed is stale vs a concurrent base write	ACCEPTED -- re-read base inside the lock
2	migration races unlocked legacy writers	REBUTTED -- pre-existing, class-wide across all ~69 update_config_locked call sites
3	base seed ignores the overlay	ACCEPTED -- overlay-first precedence
4	overlay read is outside the lock	ESCALATED -- needs a two-lock ordering discipline that exists nowhere in the tree
4	symlinked config leaves a .lock outside the data home	ACCEPTED -- containment asked about the resolved path
5	synchronous file I/O on the loop	REBUTTED -- main does the same I/O here

Six findings accepted and fixed, each mutation-verified. But rounds 2/3 demanded
opposite seed sources, and rounds 1/5 demand opposite things about the loop: round 1
was satisfied by keeping the work on the loop without waiting, and round 5 objects
to the work being on the loop at all. Under the second, the only compliant version
of this function is one that does not write -- which is main minus the migration,
not a fix for #7793.

Where that leaves the PR. Everything except this lane is green on
0dc8a392e, including all four Windows shards and Dependency Audit. The
substance of #7793 -- a migrating load() silently discarding a concurrent config
write -- is fixed, delta-scoped, and covered by a deterministic two-writer test that
is red on the base commit.

Two open decisions for a maintainer, both mine to implement once someone rules:

1. Round 4's overlay-lock residual. My recommendation stands: merge as is and close
   the class by converting the remaining direct write_config_atomically config
   writers onto update_config_locked, which its own docstring already lists as
   pending, rather than growing a bespoke two-lock protocol inside the loader. Say
   the word and I will implement the two-lock version instead.
2. This round's loop-I/O finding. I believe it is a false positive against main's
   own behavior on the same line, and I have deliberately NOT posted an
   /ai-review override myself. If you agree it is a false positive, a writer can
   override the lane; if you would rather the migration be skipped on the loop, tell
   me and I will ship that, but note it means a loop-only gateway never migrates.

test_config_loader.py 507 passed on 0dc8a392e; black and flake8 clean.

[NicholasRBowers]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause — orders the config write-back migration as a delta against the on-disk document inside the write lock, so a concurrent config write landing after the load's read survives instead of being clobbered by a re-serialized stale snapshot.

[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

• PR #4118 is OVERLAPPING relative to this PR. 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.