fix(config): route the last direct config writers through the advisory lock

[iamwhatever]

Problem / Motivation

update_config_locked takes an advisory lock on a <path>.lock sidecar for its whole read-modify-write, and its own docstring calls it "the required path for new config.json mutations". Eleven writers that pre-date it still called write_config_atomically directly and relied on the in-process asyncio _get_config_lock() instead.

That lock is a LoopBoundLock. It serializes callers on the same event loop in the same process and nothing else — not a holder of the sidecar (all ~69 update_config_locked call sites), not a worker thread, not another process. So a locked read-modify-write and one of these writers could interleave, and whichever renamed second published a document that never saw the other's change.

Concretely: kirocrew config set model opus and a kirocrew setup timezone step running at the same time, or the dashboard settings PATCH and an app uninstall's trust withdrawal. One of the two changes just is not there afterwards. Nothing errors, nothing logs, and the endpoint reports success.

Why it matters

Two things are true and neither is visible from a single call site.

Every current update_config_locked caller believes it has mutual exclusion. Against these eleven it did not — a partially adopted lock reads as safety without providing it, and the data it fails to protect is the user's whole configuration file, including inline channel credentials.

And _get_config_lock() could not be adopted from the other side to close the gap: it is an asyncio lock, so a synchronous writer, a worker-thread writer, or the CLI in a separate process cannot acquire it at all. Any fix that bridges the two primitives ends up inventing a lock-ordering discipline for one call site. Conversion is the only shape that scales.

What changed (motivation → approach → change)

Every remaining direct write_config_atomically(config_path()) caller now routes through update_config_locked, keeping each site's existing failure semantics.

All eleven were already fail-closed — each one bails rather than resetting when the config is unreadable — so every conversion takes the default on_corrupt="fail" and no site gains a reset path. stamp_meta=False at every converted site, because none of these writers stamped meta before and this change is about the lock, not the document shape.

Site	Shape before	Conversion
agents.py _commit_agent_config_locked	read+write already inside the shielded offload	synchronous update_config_locked; ConfigReadError still escapes as the unit's first step, so the handler's 500 stays exact
agents.py api_default_agent	loop-side read+write under the asyncio lock	moved into the module's own shielded _offload_config_write — the advisory acquire can WAIT, and an unbounded flock wait on the loop would stall the gateway
security.py _mutate_agent_config	own _get_config_lock + run_in_executor	overlay-owned check moves inside the mutate callback so it stays in the same hold as the write; ConfigReadError is translated to this module's ConfigCorruptError so callers keep answering a coded 409
apps/manager.py _drop_trust_grant, _restore_trust_grant	unlocked; the CLI runs both in its own process	synchronous update_config_locked. The no-grant fast path is preserved (mutate returns None, no write) and re-derived under the lock, so a concurrent revoke is a no-op rather than a redundant rewrite
cli_setup.py ×5 (whatsapp, slash command, sandbox consent, timezone, dashboard URL) and cli_chat.py _ensure_default_agent_in_config	read → prompt → write	see below

The wizard has the widest read-to-write window in the tree: it reads to compute a prompt default, blocks on the operator, then writes. The pre-prompt read stays — it decides whether the step runs and it produces the existing operator-facing messages — but it is no longer the read the write is derived from. Section shape guards are re-checked inside the lock. The sandbox-consent step's audit-then-write ordering is unchanged: the SEL event stays ahead of the acquire, so a failure between the two still leaves a record without a grant and never a grant without a record.

A mutate callback aborts on a foreign section; it never replaces one

The first revision of this PR got this wrong, and the GPT review lane caught it. A callback that assigns a fresh {} over a non-dict section destroys an operator value the step does not own and reports success — the same silent-config-loss shape this PR exists to remove, reintroduced by the fix for it. Both the slash-command and dashboard-URL sites now abort; a section that is genuinely absent is still created, which is the ordinary path.

Fixing only the callback would have shipped a dead guard. _setup_slash_command's pre-lock read does cfg.get("slack", {}).get("command", ...), which raised AttributeError on a scalar section and took the whole wizard down with a traceback before the write path was ever reached. So the read is guarded on the same rule, and that pre-existing crash becomes the clean refusal the whatsapp and sandbox steps already give.

agents.py:750 is deliberately not converted: it writes the kiro-cli agent spec, not config.json.

Three entries on the issue's list were false, and were verified against origin/main rather than taken on trust. apps/manager.py's config_local_path() use is a read-only precondition check. handlers/telemetry.py never writes config at all — git log -S finds no such write anywhere in its history.

The docstring is corrected, not deleted

The issue asks for the "legacy writers" paragraph to be removed. It is replaced instead, because deleting it would make the docstring wrong in the other direction: a second family of writers still bypasses this lock, and the ratchet below does not reach it — for two different reasons that are worth stating precisely, since the paragraph exists to mark exactly that boundary.

• Writers reaching config_path() through kiro_crew.agent._atomic_json_write (messaging.py's per-channel savers, core.py's STT PUT, mcp.py's gateway-enable) make no write_config_atomically call at all, so the matcher never sees them.
• Writers going through KiroCrewConfig.save() (updates.py's log-level PUT, core.py's theme PUT, several agents.py CRUD endpoints) do call write_config_atomically directly — but from inside loader.py, which the ratchet exempts, so the write is invisible at every caller.

That is ~16 further sites and is why this PR stops here: each needs its own failure-semantics judgement, and folding them in would triple the review surface. The docstring now names them so the next reader does not read the ratchet's green as covering them.

Tests

test/test_config_writers_advisory_lock.py (new). Drives a converted writer against a locked writer in the exact interleave that used to lose data, and asserts both changes survive — plus a canary key neither writer owns, so a whole-document clobber names what it destroyed. "Holds a lock" is not observable; "did not lose the other writer's setting" is.

Red-before proven by restoring the pre-conversion shape at each site:

Site restored	Result
apps/manager.py trust revoke	2 tests red
cli_setup.py slash-command step	red
cli_setup.py timezone step	red
slash-command / dashboard-URL foreign-section clobber	2 tests red
cli_chat.py missing stamp_meta=False	red

The two interleave cases on the wizard need no threads: the competing locked write is driven from inside the prompt, i.e. strictly after the step's read and strictly before its write. That is the worst case of the window with no timing to be flaky about. The apps-manager case uses the repo's established threading.Event pair with a bounded _TIMEOUT, so a regression fails on an assertion instead of hanging the suite. Further tests pin that fail-closed survived the conversion (an unreadable config is refused, never replaced), that a foreign section is refused in both directions (absent is still created), and that the default-agent seed stamps no meta and skips the write entirely when agents already exist.

TestEveryConfigWriterIsLocked in test/test_config_rmw_preserves_settings.py — the ratchet the issue asks for, so the list cannot regrow. Walks the AST per function, tracking names bound to config_path() / config_local_path(), so it catches a write through a local variable (path = config_path() … write_config_atomically(path, data)) and not only an inline call. Per-function scoping matters both ways: file-wide bindings would report false positives, and would also let a genuine offender hide behind an unrelated function's rebinding of the same name. config/loader.py is exempt — it holds the primitive and KiroCrewConfig.save.

It carries a self-test: a scan asserting an empty offender list is indistinguishable from a scan whose matcher is broken, so the detector is exercised on both spellings a regression would take, and on a caller-supplied path (the agents.py:750 shape) which must not be flagged.

Four existing tests took their seam on the writer this change replaces and move to update_config_locked. Every pinned property is unchanged; two are strengthened — test_default_agent_write_holds_the_config_lock now also asserts the <config>.lock sidecar was taken, which is precisely the guarantee the asyncio lock it already checked cannot give.

Manual verification

N/A — the defect is a lost update between two writers, which is only observable deterministically under an injected interleave. The new tests are that interleave; a manual reproduction would be a race and would prove less.

Screenshots / video

Why no screenshot: backend and test files only — the diff touches no frontend path and renders no pixel.

Related Issues

Surfaced by #7793 / #7937.

Pattern harvest

Rule candidate: lint

Pattern: a shared locking primitive adopted by most but not all writers of a resource — the guarantee is only as strong as the participating set, and a partially adopted lock reads as safety at every call site while providing none. Shipped as TestEveryConfigWriterIsLocked, which is the generalizable half: the defect is not "these eleven sites forgot a lock" but "nothing made forgetting it visible". A second, narrower pattern showed up during review and is worth naming: converting a bare read-modify-write into a mutate callback silently changes what happens to a value the callback does not own, so an in-place setdefault that used to crash or refuse can become an overwrite that reports success. The residual is named honestly in the primitive's docstring — the same class still exists for writers reaching the file through _atomic_json_write / KiroCrewConfig.save, and an aliased import of write_config_atomically would also evade the AST matcher.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — update_config_locked's docstring
• No secrets, credentials, or internal references in the diff

Closes #8032

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Right shape for a lost-update bug — per-site conversion under the existing primitive, failure semantics preserved, with a ratchet so the gap can't regrow.

Suggestions

• The second writer family (_atomic_json_write, KiroCrewConfig.save) lives only in a docstring; encode it in TestEveryConfigWriterIsLocked as a frozen known-offender allowlist so a new bypass writer fails immediately and the list can only shrink.
• security.py's _mutate_agent_config still hand-composes _get_config_lock + executor — the exact "third copy, free to drift" the api_default_agent hunk rejects; route it through run_config_write for the shield/drain guarantee too.

[DESIGN-REVIEWED] 0d30e91

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0d30e91

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 0d30e91dae5e31aaf6639d597be85bf0b86f18d6 — 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 claims verified: run_config_write exists with the documented both-locks contract, the sibling ratchet TestNoFailOpenConfigWriters exists, the only remaining write_config_atomically caller outside loader.py writes a caller-supplied spec path (agents.py:751, as the description says), and the second family of unlocked writers (_atomic_json_write in messaging.py/mcp.py, KiroCrewConfig.save) is real. The one finding: the ratchet's self-test re-implements the detector inline instead of exercising the real one.

First-Principles-Verdict: CONCERNS

The ratchet's vacuity self-test exercises a hand-copied matcher, not the real one — detector drift passes its own "not vacuous" check.

What this change ships

Intent: FIX — make the last eleven direct config.json writers take the advisory sidecar lock so concurrent writes stop silently losing each other.

1. App-uninstall trust revoke/restore now serialize with all other config writers — justified
2. Five setup-wizard steps write to the document as it stands at write time, not the pre-prompt snapshot — justified
3. First-chat default-agent seed takes the lock; skips the write when agents exist — justified
4. Dashboard default-agent PATCH holds both locks via existing run_config_write — justified
5. Agent-config PUT's removedTools write is one locked read-modify-write — justified
6. Trust-grant overlay check moves inside the same lock hold as the write — justified
7. Slash-command and dashboard-URL steps refuse to overwrite a non-object section — declared rider, harm named
8. Wizard no longer crashes on a scalar slack section — declared rider, pre-existing defect
9. Restore no longer duplicates a grant re-granted mid-window — rides along, same re-derive rule
10. AST ratchet forbids new direct writers; docstring names the unlocked second family — justified

Watch

• The docstring's "~16 further sites" second family (_atomic_json_write in messaging.py:5186/5524, mcp.py, plus KiroCrewConfig.save callers — verified by grep) is a counted, declared, accepted-and-deferred remainder; the ratchet's green genuinely does not cover it, exactly as the author states.

Subtractions

• Delete the inline copy of the detector in test_the_ratchet_would_catch_a_reintroduced_writer (test/test_config_rmw_preserves_settings.py): _is_config_path_call and the scope-per-function walk are spelled twice in TestEveryConfigWriterIsLocked (count: 2). Factor the matcher into one function both tests call, so the self-test proves the real ratchet sees the forbidden shape — as written, an edit that breaks the real matcher leaves its vacuity check green.

[FIRST-PRINCIPLES-REVIEWED] 0d30e91

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 0d30e91

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

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

[iamwhatever]

• fixed — BLOCKING, concurrent section updates are silently discarded (span=242b1a1fb266, src/kiro_crew/cli_setup.py) — fixed in 192ccc9c9.

section = {} / dashboard = {} at lines 787 and 1124
Concurrent config set slack|dashboard <scalar> -> locked _apply replaces the newer value -> setup reports success after losing it.
Fix: abort when an existing section is not a dict; create one only when absent.

Legitimate, and a regression this PR introduced rather than a pre-existing one. Before the conversion these two steps reached their section through dict.setdefault, which on a scalar either raised (slash command) or was caught and reported as a save failure (dashboard URL). Turning that into a mutate callback silently changed the outcome to "overwrite the operator's value and report success" — the same silent-config-loss shape this PR exists to remove.

Both sites now abort. A genuinely absent section is still created, so the ordinary path is unchanged.

One thing the finding did not name, and it matters: fixing only the callback would have shipped a dead guard. _setup_slash_command's pre-lock read does cfg.get("slack", {}).get("command", ...), which raises AttributeError on a scalar slack and takes the whole wizard down with a traceback before the write path is reached — so the callback guard was unreachable, which the first version of the regression test proved by failing with that AttributeError instead of the assertion. The read is therefore guarded on the same rule, and the pre-existing crash becomes the clean refusal the whatsapp and sandbox steps already give.

Red-before proven: with src/kiro_crew/cli_setup.py reverted to the reviewed shape, TestAForeignSectionIsNeverReplaced::test_the_slash_command_step_leaves_a_scalar_slack_section_alone and ::test_the_dashboard_url_step_leaves_a_scalar_dashboard_section_alone both fail; both pass after. Two further tests pin the absent-section direction so the abort cannot regress into refusing the normal path.

[iamwhatever]

• fixed — function-local import violates top-level-imports (span=583f63beb6ea, src/kiro_crew/dashboard/handlers/agents.py) — fixed in 677f5d16a.

function-local from ...mcp import _offload_config_write violates top-level-imports -> Fix: annotate the genuine circular import inline.

Legitimate, and now fixed by deleting the import rather than annotating it, which is strictly better than what was asked.

First pass (192ccc9c9) annotated it inline with the spelling this same file already uses at two other sites, since mcp imports agents and a module-level import would be a real cycle. The First Principles lane then observed that the surrounding composition — async with _get_config_lock() + _offload_config_write(update_config_locked, …) — is chat_utils.run_config_write. Calling the helper removes the reason the import existed, so 677f5d16a has no function-local import here at all and nothing to annotate. run_config_write is reachable as an ordinary module-level import because agents.py already imports chat_utils at line 65.

The pre-existing lazy mcp import a few hundred lines above still carries no annotation. It is outside this diff and is left alone rather than swept in.

[iamwhatever]

• fixed — omitted stamp_meta=False on the default-agent seed (span=44f906073002, src/kiro_crew/cli_chat.py) — fixed in 192ccc9c9.

omitted stamp_meta=False adds or rewrites meta during default-agent seeding, contrary to the stated shape-preservation scope -> Fix: pass stamp_meta=False.

Legitimate, and squarely my error: the commit message claimed stamp_meta=False throughout while this one site omitted it, so the PR's own stated scope and its code disagreed. update_config_locked stamps by default, and the writer it replaced stamped nothing — so a fresh install's first chat would have rewritten a key this function does not own, which is a document-shape change smuggled in by a locking fix.

Now passes stamp_meta=False. Red-before proven: with src/kiro_crew/cli_chat.py reverted to the reviewed shape, TestTheSeedWritesNoMetaBlock::test_the_seed_adds_no_meta_key fails on the "meta" not in on_disk assertion; it passes after. A companion test pins the other half of the callback contract — when agents already exists the mutate returns None and the file is not rewritten at all.

[iamwhatever]

• fixed — api_default_agent hand-builds run_config_write instead of calling it — subtraction taken in 677f5d16a.

api_default_agent (agents.py) composes async with _get_config_lock() + _offload_config_write(update_config_locked, …) inline. That exact composition is run_config_write (src/kiro_crew/dashboard/chat_utils.py:68), whose docstring calls itself "the one async entry point that holds both" … this PR adds a third instance of a composition that will diverge from the helper.
Subtraction: delete the inline async with _get_config_lock(): block and the from …handlers.mcp import _offload_config_write import; replace with await run_config_write(update_config_locked, path, mutate=_set_default, stamp_meta=False).

Taken as proposed. api_default_agent now calls run_config_write and the inline composition plus its function-local mcp import are gone — a net deletion, and it removes at the root the same import the GPT lane asked me to annotate.

I checked one thing before accepting, because it would have been a reason to decline: chat_utils.run_config_write lazily imports _get_config_lock from agents.py, so calling it from agents.py looked like it might close a two-way import loop. It does not — agents.py already imports chat_utils at module level (line 65), which is precisely why the helper's own import of _get_config_lock is lazy. There is no new dependency edge, so the objection I was weighing does not exist and the subtraction is free.

Equivalence verified rather than assumed. run_config_write acquires the same _get_config_lock(), dispatches fn to a worker thread (asyncio.to_thread), shields it in a while True drain loop, and re-raises CancelledError exactly once — the same three properties the inline _offload_config_write composition provided, so the fail-closed 500 path and the cancellation guarantee are unchanged. test_default_agent_write_holds_the_config_lock still passes unmodified, including its assertion that the <config>.lock sidecar was taken, and the seam it patches (agents._get_config_lock) is still the one the helper resolves at call time.

Gates re-run after the change: check_sync_io_in_async, check_loop_bound_locks, isort, flake8, mypy (1281 files), and 772 tests across the config / agents / apps / security / CLI families.

[iamwhatever]

• fixed — BLOCKING, concurrent re-grant is duplicated during restore (span=0251ac65b2da, src/kiro_crew/apps/manager.py) — fixed in 0d30e91da.

agent_raw["apps_trusted"] = [*(grants if isinstance(grants, list) else []), name]
Failed uninstall -> concurrent dashboard re-grant -> restore appends the existing name -> persisted trust list is corrupted with duplicates.
Fix: append name only when it is absent from the locked list.

Legitimate, and an inconsistency inside this PR's own change rather than an edge case. The pre-lock _has_trust_grant check answers "should this restore run at all"; it is not the read the write is derived from, so a dashboard re-grant landing between it and the advisory acquire is invisible to it. Re-deriving the decision from the locked read is the entire point of the conversion — _drop_trust_grant does exactly that a few lines above, and apps_trusted_local in this same callback was already using the guarded append. The base list was the one outlier, which is what makes this an oversight and not a judgement call.

It matters because apps_trusted is the durable record that decides whether a third-party app may execute, so it has to hold each name once and exactly once.

Now appends only when the locked document does not already hold the name, mirroring the apps_trusted_local branch verbatim.

Red-before proven: TestARestoredGrantIsNotDuplicated::test_a_regrant_landing_before_the_lock_is_not_appended_twice fails on the reviewed shape with ['zibble-app', 'zibble-app'] and passes after. It is driven deterministically rather than by timing — the competing locked re-grant runs from inside _has_trust_grant, i.e. strictly after the restore's pre-lock check is answered and strictly before its acquire, which is the window in its exact worst case. A second test pins the ordinary direction, so the guard cannot regress into refusing a genuine restore.

[iamwhatever]

• fixed — the docstring's opening claim contradicts the unlocked KiroCrewConfig.save call (span=15137de977ab, src/kiro_crew/config/loader.py) — fixed in 0d30e91da.

"every config.json writer that calls write_config_atomically" contradicts the documented unlocked KiroCrewConfig.save call -> Fix: narrow the claim to direct callers outside loader.py.

Legitimate, and the second instance of the same imprecision — worth saying plainly rather than patching a third time. The previous round corrected the paragraph that lists the still-unlocked family, but left the opening sentence overclaiming, so the docstring contradicted itself two paragraphs apart: KiroCrewConfig.save() does call write_config_atomically and does not come through this primitive.

Narrowed exactly as asked, and the scope is now stated once and used consistently:

The locked primitive for every DIRECT write_config_atomically(config_path()) caller outside this module, and the required path for new config.json mutations. No such caller remains …

Plus an explicit note that "direct caller outside this module" is the precise set the ratchet checks and is deliberately not the same as "every writer that reaches config.json", naming KiroCrewConfig.save as the case that proves the difference. The docstring's job here is to mark that boundary, so leaving the reader to infer it was the actual defect.

No code change; documentation only, so no test accompanies it.

[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 #7167 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 #7167: REBASE. PR #8095 rewrote the tail of the same test file on main, so the head no longer merges cleanly there; git merge-tree against origin/main auto-merges updates.py but conflicts on test/test_config_rmw_preserves_settings.py. Files: test/test_config_rmw_preserves_settings.py, src/kiro_crew/dashboard/handlers/updates.py.

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