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

[adiarora06]

Closes #7793

Problem

KiroCrewConfig.load()'s write-back migration in _load_resolved (src/kiro_crew/config/loader.py) calls cfg.save() unconditionally after detecting a legacy config shape needing migration. cfg is a snapshot from a read earlier in the same call, and nothing orders that save against a concurrent config write (a dashboard PATCH, a CLI write) landing between the migration's read and its save — the concurrent write is silently discarded when the migration's write lands last. This is reachable today because chat_runner.py already calls KiroCrewConfig.load() off the event loop via asyncio.to_thread.

The sibling publish_autocompact_pct(cfg, ticket) in the same load() already solves exactly this class of problem: it takes an ordering ticket drawn before the read, and a publish holding a ticket lower than one already published is dropped.

Fix

Extends that same ticket-ordering contract to save() itself, rather than inventing a new mechanism:

• KiroCrewConfig.save() gains an optional ticket parameter. Every existing call site omits it, so save() draws a fresh ticket at write time — which always wins. That's correct for the overwhelmingly common shape (read, mutate, save, all in one go — nothing could have raced it into being "newer").
• _load_resolved passes the ticket it already draws before its own read into the migration's cfg.save(ticket=ticket) call. If a save carrying a higher ticket has landed on disk since (the concurrent write), this save is dropped instead of clobbering it with the stale snapshot.
• The compare, the actual write_config_atomically call, and recording the new high-water ticket happen under one lock — closing the same compare-then-write TOCTOU window publish_autocompact_pct's own docstring calls out.

Migration is one-shot but idempotent: a dropped write just means this process's on-disk config stays in its legacy shape a little longer, and the concurrent write's own load (or a later one) re-detects and retries.

Why not the other two shapes from the issue

• Serializing under the loader's own lock — the migration write is reachable from a worker thread (asyncio.to_thread), off the dashboard's event-loop-bound _get_config_lock. Sharing an asyncio.Lock across that thread boundary is its own hazard, so this would need a new cross-thread primitive rather than reusing what's there. The ticket mechanism is already thread-safe (threading.Lock) and needs no new concept.
• load(migrate=False) read-only mode — a reasonable complementary idea per the issue, but unrelated API surface not needed to close this specific race. Left out to keep this PR scoped to the ordering fix.

Tests

Added TestMigrationSaveOrderingAgainstConcurrentWrites in test/test_config_loader.py:

• test_concurrent_write_between_read_and_migration_save_survives — reproduces the exact race from the issue: a load detects migration-needed, a concurrent write (its own load + mutate + save, the same shape every dashboard handler uses) lands before the migration's cfg.save() fires, and the concurrent write survives on disk.
• test_save_with_a_stale_ticket_is_dropped — pins save()'s ticket contract in isolation.
• test_concurrent_migration_saves_never_let_an_older_ticket_win — forces the compare-write-record critical section to interleave across real threads (mirrors test_autocompact_default.py's equivalent test for publish_autocompact_pct).

.venv/bin/python3 -m pytest test/test_config_loader.py -q — 499 passed, 1 skipped (pre-existing skip, unrelated).
.venv/bin/python3 -m pytest test/test_autocompact_default.py -q — 16 passed (confirms the shared ticket-counter machinery is untouched).
black / isort / flake8 / mypy clean on both touched files.

CHANGELOG.md not touched, per this repo's fix-PR policy.

🤖 Generated with Claude Code

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[dwu96]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[dwu96]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[bolichen97]

Closing — the same fix as #7937, which orders the write it misses

Verified relationship: functional overlap

Both PRs say "Closes #7793" and both rewrite the same three lines on origin/main: src/kiro_crew/config/loader.py:3262-3264, the if needs_migration and not cfg._degraded_sections: branch holding _write_migration_backup(path) / cfg.save(). #7830 turns that into if not cfg.save(ticket=ticket) backed by new in-process globals _CONFIG_WRITE_TICKET / _CONFIG_WRITE_TICKET_LOCK; #7937 deletes both lines and calls _persist_config_migration(path, frozenset(pending), default_kiro_agent=...), a re-decided delta applied inside update_config_locked(..., wait_for_lock=False). These are two incompatible designs for one requirement — whichever lands, the other is a pure conflict on the same hunk with no defect left to fix — and both append a test class to test/test_config_loader.py pinning the same race with the same _write_migration_backup suspension trick, so this is not merely shared-hub-file contact. On the merits #7937 is the right survivor and I confirmed the argument its body makes rather than taking it on faith: next_config_load_ticket has no caller outside config/loader.py (git grep on origin/main hits only loader.py:1921 and test_autocompact_default.py), #7830 bumps its high-water mark only inside KiroCrewConfig.save(), and the two writers #7830's own problem statement names — the dashboard PATCH at dashboard/handlers/core.py:2129 and kirocrew config set at cli_config.py:201 — both write through update_config_locked and draw no ticket, so my_ticket < _CONFIG_WRITE_TICKET is never true for them and the stale whole-snapshot write still clobbers the PATCH. #7830 is also in-process-only module state, so it gives zero ordering against the cross-process "CLI write" the issue names, whereas update_config_locked holds a real <path>.lock advisory flock and main's own docstring (loader.py ~1030) already calls it "the required path for new config.json mutations". #7937 is additionally strictly broader on the same defect — the write touches only agents/default_agent/workspaces so any other setting is untouchable by any writer class, it is idempotent (a migration another writer already did writes nothing and leaves no redundant .bak), it extends the existing .bak containment predicate to the new .lock sidecar via _inside_data_home, and it declines instead of stalling the loop. Review state points the same way: #7937 has design-review PASS, Opus "no blocking findings", advisory first-principles CONCERNS and an author answering findings, while #7830 has the review thread == [[]]and four repeats of the PR-template hygiene bot, so its workflows are not even auto-approved. Neither change is on main and neither branch is stacked (one commit each,15591fcoffbe2ee94and0f73920off37d2b6b`).

This was adjudicated twice, independently; the second reviewer reached the same ruling (functional overlap). Their strongest corroborating fact:

(1) Conflict anchor: origin/main src/kiro_crew/config/loader.py:3264 is cfg.save(); #7830 replaces it with if not cfg.save(ticket=ticket): while #7937 deletes it plus the preceding _write_migration_backup(path) for _persist_config_migration(path, frozenset(pending), default_kiro_agent=cfg.agent.default_agent or "kirocrew"). (2) #7830 cannot see its own named writers: _CONFIG_WRITE_TICKET = my_ticket appears once, inside save(), but dashboard/handlers/core.py:2129 (config PATCH) and cli_config.py:201 (kirocrew config set) write via update_config_locked → write_config_atomically, and next_config_load_ticket has no caller outside config/loader.py on main. (3) Process scope: #7830 uses threading.Lock + a module global (in-process only); #7937 routes through update_config_locked's <path>.lock advisory flock, which main's own docstring calls "the required path for new config.json mutations" and which serializes across processes.

Evidence

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 and not the other

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.

Carry this over first

This closure is about redundancy, and these items are the exception: they are not on main and not in the surviving PR, so they need a home before the topic is finished. Please don't let them go with the branch.

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. Findings that implied a closure were re-adjudicated independently, including an adversarial pass whose only job was to refute them; the reasoning above is what survived. If it is wrong, reopening costs nothing — please say so, and treat the reasoning rather than the outcome as the thing to correct.}