Repository navigation
fix(acp): keep a chat's effort pick from leaking into other chats - #14528
unstablebrainiac wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS No design problem blocks this. Kiro-cli re-reads the file's level when a session switches model or mode, so dropping the shared file isn't an option; recording which entries Kiro Crew wrote and pushing each chat's own level with Notes
[DESIGN-REVIEWED] 2dfd5d9 Description read[DESCRIPTION-READ] ecbe6ea5442cba5f383b1967851ac7b94b06090dd6ff76dbec006527d679f34f That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS The fix goes after the real cause, and the description names the remaining cause it leaves for later (a per-session overlay). But the PR also carries a separate defect fix (#23396) that grew its own chain of writer changes. Notes
What this change shipsInventory (10 items) — 7 justifiedIntent: stop one chat's effort pick from silently running in other chats in the same work dir, and make Default actually run the model's default. FIX.
[FIRST-PRINCIPLES-REVIEWED] 2dfd5d9 Description read[DESCRIPTION-READ] cceb45936028e86bb69f308ccf07946114c642c2c4d8d34f345360a1673c2979 That sha256 names the PR description this verdict read. Recompute it with |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
aff3cc3 to
5558f86
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5558f86 to
514798b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
514798b to
c46b100
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
HOLD at c640ad3. This head is a rebase of a777826 plus one test-import change in The new lane run on this head raised items that have no reply yet:
The blockers from r116 still stand, and only a maintainer can clear them. The PR adds a — crew-pr-reviewer (r117) |
|
|
HOLD at dd83aac. This head adds two things since r117: a rewrite whose encoded size would pass the 1 MiB ceiling is refused before anything is written, and What blocks approval:
Our earlier request for changes stays until these clear. I read the interdiff since r117, not the full diff, and did not run tests. — crew-pr-reviewer (r120) |
|
|
|
|
|
|
|
|
|
HOLD at dd83aac, reserved for human security review. The head is unchanged since r120. Every current-head lane note now has a What still blocks approval:
I found no new defect. Our earlier request for changes stays until these clear. I read CI and the replies, and did not run tests. Reviewed head: dd83aac — crew-pr-reviewer (r121) |
|
HOLD at 2e3d7d6, reserved for human security review. The root-cause fix still holds: What blocks:
I read the interdiff since r121, the new lifecycle code, the lanes and CI. I did not run tests. — crew-pr-reviewer (r123) |
|
|
|
|
|
|
Hold at The root-cause fix still holds, and GPT 6.1 and Opus report no findings on this head. What blocks:
— crew-pr-reviewer (r124) |
A kiro-family session applies reasoning effort through <work_dir>/.kiro/settings/cli.json, one file shared by every session in that work dir. Each new AcpProvider read that file back into its own per-model effort map, so a level one chat picked ran in every other chat of the same project while their pickers showed Default, and choosing Default could not bring the model's own default back. The provider no longer reads the overlay back. Before each spawn it projects its own resolved level: written when one resolves, and the model's entry removed when none does. The removal checks the file first and takes the lock only when there is an entry, so a directory with no overlay is not given one. Constructing a provider does no file I/O: start() writes both overlays off the event loop before every spawn. A Default or auto spawn keeps a level Kiro Crew did not write and warns about it; the warning names a value only when it is one of the five effort levels, and redacts each model name whole before cutting it, so no other text from the file reaches the gateway log. Every overlay read refuses a link, a hardlink, a file renamed over the name after its open, or a non-regular file and reads through the descriptor-checked containment helper, and every write is published through the settings directory opened without following links where the platform supports descriptor-relative writes. The settings directory is resolved from <work_dir>/.kiro/settings one name at a time, with lstat and readlink only on names inside the resolved work directory: a .kiro or settings link is followed when its text names a folder inside the work directory, and refused unopened when it names an outside, remote or other-drive path or loops, since a repository can ship .kiro -> ~/.kiro or a link to a network share. The lock-free cli.json existence checks and the spawn fence key take that same admitted folder, so no step stats a refused link's target. test/link_screen_sites.py declares workspace_cli_settings.py::_settings_dir_within_work_dir, which screens each name with platform_compat.is_link_or_junction before reading its link text. The directories down to the admitted folder are walked by those names without following a link, so a part swapped for a link after admission is refused. The settings lock is created and opened through that same pinned directory, and refuses when the folder reached by name is no longer that directory. Where descriptor-relative writes are unavailable (Windows), the lock creates and holds each directory on that chain before it creates the next, never creating through a folder it has not checked, and holds them all open until it is released, so none can be renamed or replaced while the overlay is read and written; it refuses a held folder that has taken a reparse point in place. The lock resolves the work directory once and refuses when a later resolution names another directory, so a retargeted work-directory link cannot move the lock or the write into another workspace. The in-process spawn fence is keyed on the admitted settings folder, so two work directories sharing one exclude each other, and a work directory that cannot be resolved fails the start. The link screen in providers/acp.py::_read_cli_overlay_document refuses every link at the cli.json name and never traverses one, so it needs no hold; the helper read bounds a link planted after the screen to the settings directory. That reader decodes with utf-8-sig, so a cli.json saved with a byte-order mark parses and is written back as plain UTF-8 (kirodotdev#16251); test/test_acp_cli_overlay_bom.py pins that through the overlay writers and the explicit clear, since no overlay read-back remains. Only the explicit Default pick may remove a level Kiro Crew did not write, and it refuses a cli.json whose chat.modelDefaults, model entry or active effort key for that model is not an object, leaving the file unchanged. A live clear whose /effort push fails after its rewrite asks for a reset, since the file already holds the default. When the pick cannot do that itself (no live session, a turn in flight, a clear that raised, a model without effort), the session map carries a one-shot explicit_effort_default flag to the session's next cold start, because that start, not the pick, resolves the work directory and the model whose entry it removes, and either can change after the pick. That start skips the warm pool, removes the entry for the concrete model it runs when that model takes effort, and clears the flag once its projection succeeds; an auto session or a model without effort spends it removing nothing, as the live clear does. The pick saves the flag before it answers, and a start saves it spent before running the replacement, so the replacement runs at most once. A level that replaces a pending flag saves that before its live push and puts the flag back if the pick does not commit. While a session's cold start is in flight, an effort pick that would change that session's flag is refused with the same retryable 409 a turn in flight gives, so no pick changes the basis a start publishes on; a level pick with no Default pending changes nothing there and lands. A pick already saving its flag when the start begins is waited for before the start reads it. The session map holds the flag for at most 1,000 sessions, each key at most 200 characters; a pick it cannot hold is refused with 503 before anything changes, and a flag cleared on a row that holds nothing else takes the row with it. A persisted flags value that is not an object reads as no flags and is replaced when a flag is set or cleared on that row, and only a stored true counts as the flag. A start or a pick that clears a session's flag holds its row until it ends, so arming the flag again needs no new row and no other chat's pick can take the row meanwhile. A re-arm whose save fails fails the start rather than publish it, and the next start spends the flag it left pending; a refused pick whose rollback cannot save says so. A pick whose intent save fails after its push reached the live session answers 503 with the slot on the level that session runs, and the same pick again retries the save. Every explicit Default that rewrites the file, live or at that start, does it inside a warm-pool fence: no warm runtime is claimed until the write settles, each one queued before it is retired then, and one claimed before the write began is discarded before its session is registered, since each may have read the removed entry at its own spawn. A start or live clear cancelled mid-write lowers the fence only once the write settles, and the start still records whether its replacement ran. Inside one gateway, a spawn that leaves its model at the model default holds a per-work-directory fence from its projection until its runtime start returns, since which protocol step reads the file is not established. Such spawns share the fence. Every write that can put a level into the file waits for them, up to 30 seconds for a pick or a clear and 2 for another spawn's projection, and takes the answer a held lock gives when that wait runs out. A pick whose live push fails puts the prior level back with the same 30-second lock wait. A spawn waiting for the fence holds off new writes, so it waits only for a write already under way; if that write outlasts the longest file-lock wait, the spawn fails its start rather than read a file the write could still change. The fence key is the work directory's real path, resolved in a worker thread so a slow or wedged workspace cannot stall the gateway loop; the registry lookup that follows stays synchronous, so two chats cannot get two fences for one directory. A test fails if any async effort path does work-directory filesystem I/O on the loop thread.
|
|
|
|
|
Problem / Motivation
Goal: A chat's reasoning-effort pick applies to that chat only, and Default runs the model's own default.
On the kiro backend, every chat in one work directory shares one effort file,
<work_dir>/.kiro/settings/cli.json. When one chat picked a level, every other chat in that directory ran it from its next start, while its effort picker still read Default. Choosing Default there changed nothing, because the chat was already at Default. Only the chat that set the level could clear it, and only while its session was running.Seen live: three older chats had once picked Max for Opus 4.7, 4.8 and 5.5. The shared overlay held Max for all three models, and a newer chat whose picker showed Default was running at Max.
Main also follows a
.kiroorsettingslink out of the work directory. Where.kirolinks to~/.kiro, a pick overwrites the user's global kiro-cli level for that model and a Default deletes it (#22821). This PR refuses such a link: its ownership record, the record's stamp and an explicit Default carried to the next start would otherwise act on the user's global settings too.Why it matters
Effort sets cost, latency and depth. A level picked in one chat silently changes every other chat in the same project, and the picker hides it. Settings → Default Reasoning Effort is overridden the same way, and setting it back to Default does not undo a level that is already in the file.
Not a goal
kirocrew.effortOwnedandkirocrew.effortOwnedStamp, which kiro-cli loads without complaint (Manual verification).cli.jsonand relying on the/effortpush alone. kiro-cli re-applies the file's level, and drops a pushed one, when a live session switches model or agent. Measured on kiro-cli 2.29.0 over ACP: after/effort high, the level came back as the file'slowaftersession/set_model(away and back, or to the current model) and aftersession/set_mode. With no entry in the file, the next request after either switch carried no effort at all./clear,/compact,session/loadand rewrites of the file kept the pushed level.spawn_continue. That path never records the level, and only picked it up by accident when the run's directory survived.What changed (motivation → approach → change)
/effortautoautospawn may run a level Kiro Crew did not writecli.jsonpath/effortafter the spawn and after each fresh warm-pool conversation/effortpushcli.jsonand takes its level as theeffortLevelconfig option (main's #18630)effortLevel, and writes no overlay; the post-spawn/effortpush skips it, since KAS answers that command with -32603autospawn does; the chat's own level is bound to the served model and pushed with/effortafter each spawn and each fresh warm-pool conversationexplicit_effort_defaultflag on the session: the next cold start skips the warm pool and removes the entry for the model it runs; a pick that would change the flag during that start answers the retryable 409turn_in_flight, and past 1,000 flagged sessions a pick answers 503, both before anything changes; its lifecycle is the last rowsession_set_modellevel is committed at the turn start while a Default is pending (main's #15422)cli.jsonis larger than 1 MiBautospawn warns, and the picker reports the 1 MiB limit; a pinned session's own level still arrives by/effort, while a level picked with no model pinned is logged and not applied, because its first push goes through the refused writecli.jsonholding non-ASCII text, such as CJKcli.jsontoo deep forcopy.deepcopybut not forjson.loadsjson.dumpsraisedjson.dumps's depth it leaves the file, logs once and uses authored agentscli.jsonis a link, a hardlink or not a regular file, or a.kiroorsettingslink names a folder outside the work directory.kiroorsettingslink names a folder inside the work directory that is on the sensitive-path list, such as~/.awswhen the work directory is the home directoryautospawn logs that it may run another session's levelsettings --workspace, a hand edit, a second gateway) changescli.jsonafter Kiro Crew read it and before it publishesexplicit_effort_defaultflag of the explicit Default row, in fullexplicit_effort_defaultflag on the session (session_map.py,session.py), carried to the next cold start because that start, not the pick, resolves the work dir and model; the start skips the warm pool (session_allocation.py,session_allocation_parts/get_or_create.py), arms the provider (providers/base.py), and, on kiro-cli, the one harness that reads the file, removes the entry for the model it runs, removing nothing for a model without effort, as the live clear does (docs/system-specs/modules/session.md); with no model pinned, orauto, it removes that entry once the backend names the model and restarts a fresh session once if that session read it, while a session kiro-cli restored withsession/loadkeeps the level it last ran at until its next fresh session; the flag stays saved until that removal has run and is cleared only then, so a crash before the clear is saved leaves it for the next start, a clear whose save fails does not fail the start, and only one of two starts on the chat arms it; while that start runs, from its read of the flag until it registers the session, a pick that would change the flag answers the retryable 409turn_in_flightand changes nothing (a message queued behind a live turn is not a start and refuses no pick), and a start that begins while a pick is still saving reads the flag only after that save settles; an eager prefetch takes the chat's switch lock before it reads the slot's level and holds it through that allocation, so a pick waits for it instead of landing between them; a start on a level its caller chose runs that level and leaves the flag for the next start without one; at most 1,000 sessions hold it, a pick past that answers 503 before anything changes, a cleared flag leaves no empty row behind, and a pick that clears a flag keeps its row until it ends; every explicit Default rewrites the file inside a warm-pool fence (session_pool.py): no warm runtime is claimed until the write settles, and each one queued before it is retired then, also when the start or clear is cancelled mid-write; one claimed before the write began is discarded before its session is registeredA chat on Default effort ran another chat's level because
AcpProvider.__init__read everychat.modelDefaultsentry from the overlay into_effort_per_model. The read was meant to survive a gateway restart, but the file belongs to the whole work directory, so the level it found was often another session's. A dashboard slot already restores its own level from saved metadata, so the read added only other sessions' picks.The provider no longer reads the overlay back.
start()runs_apply_effort_overlaybefore every spawn. The same file holds kiro-cli's operator-editable workspace settings, so Kiro Crew only changes what it wrote itself._write_cli_overlayrecords its{model: level}in a flatkirocrew.effortOwnedkey inside the samecli.json, so the level and its record land in one atomic write. Each write also storeskirocrew.effortOwnedStampand sets it as the staged file's modification time before the rename (mtime_nsinatomic_write.py). Kiro Crew owns a value only when the record names that exact level and the stamp still matches the file, so any outside rewrite voids the record. An automatic projection never replaces or claims any other value, including a JSONnullor a non-object, and drops only a stale or malformed record. A clear removes an entry only when the record names that exact level, and leaves achat.modelDefaultsthat is not an object as it is. Only the dashboard's explicit Default pick, indashboard/chat_handlers.py, removes an entry whoever wrote it: at once on a running chat, otherwise at the session's next cold start. With a Kiro Crew default level set, it swaps the entry for that default in one atomic write.The file is shared, so a picked level travels live: after each spawn, and after each fresh conversation on a warm runtime,
_reassert_projected_effortpushes it with the same/effortcommand the picker uses. Before a Default orautospawn, one warning names up to 10 levels left in the file that Kiro Crew did not write, and counts the rest; each is escaped and cut to length. Both overlay writers read the file through_read_cli_overlay_document, which reads at most 1 MiB (_CLI_JSON_MAX_BYTES): acli.jsonthat is a link, hardlinked, larger, unreadable, unparseable or not an object is left byte-for-byte unchanged and the write raises, which the provider logs while the session still starts.locked_workspace_cli_settingsinworkspace_cli_settings.pyopens the settings folder without following links and takes the lock there; the read and_publish_cli_overlayuse that folder, and the write raises if.kiro/settingsthen names a different folder. The clear uses the same reader, treating an unparseable file as naming no effort and failing on a refused one. On the event-loop thread the settings lock makes one attempt, sostart()runs the projection throughasyncio.to_thread, and a cancelledstart()waits for that worker so a late write cannot land after another session's projection. Skill projection and Code Review Sage keep a valid record valid when they rewrite the file; any other writer voids it. Comments inproviders/acp.pyanddocs/system-specs/modules/providers.mddescribe the file as a projection, not a store.Known limitation
The overlay is still one file per work directory. Inside one gateway, a spawn that leaves its model at the model default holds a work-directory fence from its projection until its runtime start returns, so no level can land in that window: not a chat's pick, not a live Default clear that writes a workspace default, and not another session's level projection. Spawns that leave their model at the default share the window, because each only clears. Such a spawn first waits, holding nothing, until no write is held or queued in its work directory, then queues for its cold-start permit holding nothing on the fence, and takes the fence only once it has that permit and no write is held or waiting; otherwise it hands the permit back and waits again. So a start that is only queued never holds off a pick, and a start waiting on a fence never holds a gateway-wide admission slot; the permit does cover its projection and Tool Search write, each at most the 2 s startup lock wait. If writes keep it waiting past the longest file-lock wait plus that 2 s margin, the spawn fails its start rather than read a file a write could still change. A pick that meets the fence waits, up to the same ceiling it already waits for the file's lock, and then takes the same answer a held lock gives it. A pick on a chat whose eager prefetch is starting its session waits for that start to register instead of answering 409, because the prefetch holds the chat's switch lock from its read of the slot's level through its allocation. The fence is in-process and ends with the whole runtime start, its
session/newand that request's own gate wait included, because which protocol step reads the file is not established: a second gateway on the same work directory, kiro-cli's ownsettings --workspacewrite, Code Review Sage and a hand edit are not held by it, and a level one of those writes between the projection and the read is still run by that session until its next spawn, because kiro-cli has no live "model default" to push. Levels written by an older build carry no ownership record, so they read as hand-written: they are not removed, and a Default orautospawn warns about them. To clear one, pick a level in the effort picker and then pick Default (on anautochat, select the listed model first), or delete the entry fromcli.json; a slot that already shows Default does not reach the clear. When the operator's level is kept, a chat's own pick rides only on the/effortpush, so a failed push leaves that session at the operator's level; the failure is logged. kiro-cli 2.27.1 lists/effortas deprecated in favour of its/modelsettings panel. Main's live picker change already relies on it, and this PR uses it to carry a pick past a kept operator level; moving both onto the replacement channel is tracked in #14840. Any outside rewrite ofcli.json, including kiro-cli's ownsettings --workspacewrite, voids the ownership record, so Kiro Crew keeps its own levels after it and the Default orautospawn's warning names them. The ownership record depends on kiro-cli loading a workspacecli.jsonthat carries two keys it does not know, and on its own writes moving the file's mtime off the stamp. Both hold on kiro-cli 2.27.1.test/test_kiro_cli_effort_ownership_pin.pyfails if the bundled kiro-cli pinned inpackaging/kiro-cli-versionis raised past it; a kiro-cli the host installed itself is not checked. If a later kiro-cli drops the key, every entry Kiro Crew wrote reads as the operator's: nothing is deleted, but a Default orautochat can run a stale level, and that chat's pre-spawn warning names it.docs/reference/kiro-cli/README.mdnames the checks to re-run after each kiro-cli update: kiro-cli still loads such a file, its ownsettings --workspacewrite moves the mtime off the stamp, and a live/effortpush still sets the session's level. On Windows, which has no descriptor-relative writes, the lock creates and holds each folder from the work directory down to the admitted settings folder open before it creates the next, and the write and the lock go by name while they are held, so none can be renamed or replaced until the write finishes; a part that becomes a link after admission is refused. A held folder that takes a reparse point in place is refused at the next check, andsettingsis refused both before and after the lock file opens; while a folder is still empty, a reparse point set in the instant before that check can still createsettingsor the lock file outside the workspace, and the lock then refuses without reading or writing the overlay. Closing the gap the fence leaves needs a per-session overlay or effort carried insession/new, which is outside this PR and tracked in #22496. A crash after a start's removal ran but before its clear was saved makes the next start run the removal again. It normally finds the entry already gone, but a level written by hand in between is replaced, as one written between the pick and the first start is. A level pick whose intent save fails answers 503 and leaves the Default it was clearing pending, so a later start on that chat without its own level, such as an alias slot on Default, applies it; picking the level again retries the save. A Default left pending because a start ran another slot's level on the same session keeps a level pick behind the retryable 409turn_in_flightwhile a second message waits for that session, until the wait ends. When two cold starts on one chat race and the one that applies a pending Default loses registration, the winner keeps the level it started with until its next spawn; the Default's removal fromcli.jsonhas already run, and the start that ran it cleared the flag, so it does not run again.Backwards compatibility
Breaking: a
cli.jsonthat is a link or a hardlink is refused instead of followed, and so is a.kiroorsettingslink whose text names a folder outside the work directory, another drive or a network share, or loops; a link to a folder inside the work directory is followed. The decision is made from the link's own text, so a refused target is never opened. In such a work directory the effort and Tool Search writes raise and are logged, an effort pick or Default reports that it could not change the file, and a Default orautospawn warns; the session still starts. Skill projection and Code Review Sage take the same settings lock, so in such a directory skill projection is skipped with a logged warning and the session uses its authored agents, and a Code Review Sage worker runs without its effort overlay, also logged. No Kiro Crew code creates these links (writer sweep onc24c7bc8d). A.kiroshared across worktrees points outside each work directory and stays refused: a repository can ship.kiro -> ~/.kiro, which would put a chat's level into the user's global settings. Otherwise nothing main accepts is rejected, with one exception: when a Kiro Crew default level is set, a Default pick over acli.jsonwhosechat.modelDefaults, model entry or active effort key for that model is not an object leaves the file unchanged instead of replacing that value as main did. A running chat's pick answers 409effort_overlay_busy; a stopped chat's next cold start logs the refusal and keeps the pick pending. A Default pick, or a level pick over a pending Default, on a chat whose session a cron or channel turn is starting answers the same 409turn_in_flighta running turn gives, and lands on retry once the session is up; a level pick with no Default pending lands at once. A chat keeps any level it picked itself. Hand-written effort entries in a project'scli.jsonare preserved, including an effort under the other model family's key. Upgrading: a level an older build wrote has no stampedkirocrew.effortOwnedrecord, so it is kept like a hand-written one and a chat on Default keeps running it; the Default spawn's gateway-log warning names it. Picking a level and then Default on that chat removes it for that chat's model, on a stopped chat at its next cold start. No one-time cleanup runs at upgrade: an older build's entry and an operator's are the same bytes, so a cleanup would delete operators' levels too. Kiro Crew adds two keys to that file,kirocrew.effortOwnedandkirocrew.effortOwnedStamp, and sets the file's modification time to the stamp; kiro-cli ignores both keys. A clear that removes Kiro Crew's last entry drops both keys. Deleting them by hand makes Kiro Crew treat its own levels as hand-written, like an older build's. A write that would change nothing but a valid record's stamp is skipped, and a file with no record gets no stamp, so a same-level spawn or a repeat Tool Search write leaves a committedcli.jsonbyte-identical, as on main.Also taken away: an effort or Tool Search write that finds
cli.jsonunparseable or not a JSON object now raises and leaves the bytes as they are, where main reset the file to{}and overwrote it. An automatic clear removes only the entry under the model's own family key (output_configorreasoning), where main swept both; an explicit clear still sweeps both. Construction no longer writes the Tool Search overlay either:start()writes it after the effort projection and before the runtime starts, so a provider that is built but never started leavescli.jsonuntouched.Readers of what this PR takes away, found by grepping
origin/mainat99c76a43d; a test pins each:cli.jsonnested past Python's recursion limit is left unchanged the same way (theRecursionErroris caught);test/test_native_skill_projection.py::test_projection_lock_refuses_planted_symlink,::test_projection_lock_failure_keeps_aliases_and_preserves_startup,::test_kiro_link_inside_the_work_dir_projects_into_its_target,::test_kiro_link_leaving_the_work_dir_skips_projectionasyncio.to_thread, so waiting on the lock does not block the event loop;src/kiro_crew/apps/builtins/code_review_sage/tests/test_review_pool.py::TestReviewEffort::test_write_effort_overlay_never_raises,::test_a_settings_link_leaving_the_work_dir_gets_no_writetest/test_session_work_dir.py::TestReclaimRule::test_a_directory_holding_an_effort_projection_is_removedAcpProvider._apply_tool_search_overlay) -- chat -- a refused Tool Search write is logged and the spawn continues;test/test_acp_tool_search.py::TestApplyToolSearchOverlay::test_unmergeable_file_survives_construction_and_start,test/test_native_skill_projection.py::test_settings_link_outside_is_never_probed_by_any_prelock_pathturn_in_flight, 409effort_overlay_busy, 503effort_intent_unavailable) writes nothing to the store;website/src/lib/slotSwitch.test.ts"rejects on request failure and writes nothing (no earlier success to adopt)"turn_in_flight;website/src/test/agentSwitchFeedback.test.ts"maps a 409 turn_in_flight to the specific retry-later copy"chat.modelDefaultsback from the overlay, so a session starts on its own level: a dashboard slot's saved level,ConfigLoader.resolve_session_effortfor a channel thread or cron job, the spawn's own level for a sub-agent;test/test_effort_shared_overlay.py::TestTheSharedOverlayIsNotReadBack::test_levels_for_other_models_are_neither_adopted_nor_removedAcpProvider.clear_effort) -- chat -- an automatic clear (a live model switch on a Default slot) keeps an entry Kiro Crew did not write and pushes the default live;test/test_effort_shared_overlay.py::TestTheSharedOverlayIsNotReadBack::test_operator_authored_effort_survives_a_live_switch_on_defaultMain's #21009 split
session.pyinto owner modules, so the flag's manager methods live insession_parts/turn_context.py, the in-flight intent-write record insession_parts/record.py, and the warm-pool fence delegators insession_parts/runtimes.py;session.pyonly binds them.Tests
test/test_effort.pyand its siblingstest/test_effort_*.pybuild real kiroAcpProviders on temp work dirs, the path the leak went through. Tests that fail on main:assert 'max' is Nonefails);cli.jsonuntouched, and a later spawn projects again and removes a level written before it;autosession removes the levels Kiro Crew wrote for every model;autopeer and a same-model Default peer) does not change what the session runs:/effort maxis pushed after the spawn.Tests that pin what must not change: a live explicit clear whose push fails after its rewrite asks for a reset rather than arming the flag, while an automatic clear still raises; a flag cleared on a chat that never ran leaves no row behind; a clear whose save fails after the removal ran does not fail the start, and the next save writes it; a refused pick whose rollback cannot save says so; a pick whose intent save fails answers 503
effort_intent_unsavedwith the slot on the level the live session runs, the same pick again retries that save, and a matrix over the handler's exits pins that no answer reports success after a failed intent save; an explicit Default over acli.jsonwhosechat.modelDefaults, model entry or active effort key is not an object refuses, leaves the file byte-for-byte unchanged and answers 409effort_overlay_busy; no warm runtime is claimed while an explicit Default's rewrite is in progress, nor one queued or spawned before it settled, one claimed before a rewrite begins is discarded at registration and the start goes cold, and a start or live clear cancelled mid-write lowers that fence only once the write settles, the start still recording that its replacement ran; an operator-authored entry survives a Default session and anautosession; an entry the operator changed after Kiro Crew wrote it survives and the stale record is dropped; the level and its record are published in one write, and a failed write or clear leaves both as they were; a malformed ownership record owns nothing and a malformedcli.jsonis left unchanged; anautosession keeps achat.modelDefaultsthat is not an object while it drops a stale record; an operator value equal to the projected level is not claimed, even under the other family key; an unowned JSONnullsurvives a projection and an ownership-guarded clear byte for byte, and a record entry ofnullor""owns nothing on either path; an owned entry the operator removed and later restored by hand survives; a live model switch on a Default slot keeps a hand-written level while the explicit Default pick still removes it; the session's own level replaces a leftover; neither a session on Default nor anautosession creates.kiro/in an empty directory; a Default session with no workspace default pushes nothing; a rejected/effortpush does not break the start; a clear that loses the lock returns False and leaves the file; a cancelledstart()waits for its overlay worker before it re-raises, also when it is cancelled again while it waits. That test and the lock-holder test start their task or thread insidetry/finally, so a failed assertion still releases and joins it. For the kept-value design: an operator'slowsurvives a chat that projectsmax, and a later automatic clear keeps it; a stale record over a changed value is dropped without touching the value; a present non-objectchat.modelDefaults, model entry or effort entry survives a projection; an effort under the other family key survives a projection; the explicit Default pick with a Kiro Crew default level replaces an operator entry with that default in one write, removes the other family key's effort and keeps sibling keys, rebuilds achat.modelDefaultsthat is not an object while an automatic projection keeps it, and a failed write leaves the file byte-for-byte unchanged, restores the override and pushes nothing; no automatic path (a projection,change_effort, an automatic clear) replaces a value Kiro Crew did not write; an automatic clear with that default keeps the operator entry and pushes the default live; a fresh warm-pool conversation pushes the session's level and nothing for Default orauto; a Default orautospawn warns once about a level Kiro Crew did not write and stays silent when none is left, when only another model's or the other family key's value remains; failure warnings state the consequence, not a cause. Acli.jsonholding an integer past Python's 4,300-digit limit, or nesting past the recursion limit, reads as unparseable: a clear, a Default orautoprojection andstart()all leave it unchanged, and a write raises without touching it, while a failed read still reports failure. The unowned-level warning and the kept-value debug line name a value only when it is one of the five effort levels and print<unrecognized>for any other, so no other text from the file reaches the log and a JWT-shaped value or model name leaves no fragment there. Model names are escaped, so a newline or U+202E in a key cannot start a new log line, and are redacted whole before the 120-character cut; containers render as a placeholder, and the warning lists at most 10 entries with an exact count of the rest.src/kiro_crew/security_posture.pylistsproviders/acp.pyamong the modules whose redaction feeds a local log line rather than an egress path, which the redaction call-site check intest/test_security_posture.pyrequires of every module that calls a redactor. A clear that removes levels Kiro Crew wrote logs one info line naming the file and each removed level, escaped and bounded like the warning. Both lines build their list in one collector that stores at most 10 entries, each escaped and cut, and counts the rest: fed 500 pairs of 100,000-character values, it keeps 10 and counts 490. Acli.jsonpast 1 MiB is left byte-for-byte unchanged: an effort write raises, a clear returns False and logs the ceiling, and anautospawn logs that it may run another session's level. Over the same file,change_effortraises and the explicit Default returnsNone, each keeping the session's level and naming the ceiling, and the dashboard's 409effort_overlay_busynames the 1 MiB limit.test/test_kiro_cli_effort_ownership_pin.pyfails whenpackaging/kiro-cli-versionpins a kiro-cli newer than 2.27.1. Acli.jsonthat links to a file inside or outside the settings folder, dangles, or is a hardlink, and a.kiroorsettingslink whose text names a folder outside the work directory, are refused: the effort write and the Tool Search write raise, the clear returns False, and neither the link target nor any workspace file gains its bytes. A.kiro/settingslink to.awsin a work directory that is the home directory, by a relative or absolute target and on the pinned and by-name paths, is refused before the lock or any read: the effort and Tool Search writes raise, the clear returns False, and.awskeeps its bytes and gains no lock or file; the pinned reader refuses a sensitivecli.jsondescriptor before reading it.test_settings_link_outside_is_never_probed_by_any_prelock_pathrecords everylstat,stat,open,readlink,scandir,listdirandrealpathacross the effort, Tool Search, clear, Default, fence-key and skill-projection paths with.kiroorsettingslinked outside, and finds none at or below the outside folder.test/test_native_skill_projection_pinned_settings.pyreplaces the settings folder with a link to an outside folder after the lock admits it, once before the projection's write and once before its rollback, and checks that the outside file is unchanged and the projection is skipped with its logged line. The same file plants acli.jsonthat is a hardlink, or a symlink to an outside file, and checks that the projection and its rollback skip with the refusal logged, the file and its target unchanged, and no alias published, on the descriptor path and on the by-name floor alike. On the descriptor path, acli.jsonthat changes during every read skips the projection the same way. The fence, fence-order, shared-overlay and Tool Search test files requestno_zero_tool_check, so a provider whose runtime start is stubbed never starts its default client after the start. A link to a folder inside the work directory, by a relative or absolute target or through a..that stays inside, is followed: the writes and the lock land in its target and skill projection projects there; a..that escapes, an absolute target outside, a loop and a non-directory part are refused, and Windows device and UNC spellings of a target are normalized and refused by text alone. A work directory that resolves elsewhere between the lock's two resolutions is refused without a crash. A replacement renamed overcli.jsonright after the pinned read opens it is refused as well: the write raises, the clear returns False, and the replacement keeps its bytes. A settings folder swapped for a link, or replaced by a new folder, right after the read cannot take the write: the effort write, the Tool Search write and the clear all fail, the folder now at that name keeps itscli.jsonbytes and gains no sibling, and the moved folder is not written. The same holds for a folder replaced before the read, whosecli.jsonis never read, and for one replaced after the write's own check. A.kiroorsettingsfolder swapped for a link right after the lock's own check makes the lock raise, and the folder the link points at gains no lock file or directory. The same holds for a swap after the lock has opened the folder, including one that pairs the swap with a hardlinked twin of the lock file, and a settings folder removed at that point is reported as removed. A hardlinked-twin swap while the lock is being taken makes it raise too. A lock whose create loses to a sibling's create opens and locks the sibling's file instead of failing. On the by-name path Windows uses, run on Linux with that path forced, a swap right after the lock's first check makes it raise with nosettingscreated in the linked folder, a fresh work dir still gets.kiro/settingswith neither folder a link, a swap right after the lock's second check makes it raise with nothing created in the linked folder, or, where the held.kirocannot be removed, locks the work directory's own folder with nothing created outside, a swap after the folders are held makes it raise at its folder check, and.kiroandsettingsstay open through the lock file's open and the write and are closed after. A work directory reached through a link that is retargeted at another workspace while the lock is taken never reaches that workspace: a retarget before the lock's second resolution is refused, one after it keeps the lock and the write in the first workspace, and the other workspace'scli.jsonkeeps its bytes; Windows runs both with a junction. A reparse point taken in place by the held.kirofolder is refused beforesettingsis created in it, or once it is, and one taken by the heldsettingsfolder is refused before the lock file opens and after it; both are simulated through the stat attribute. Over a linkedcli.json,change_effortraises naming the link and changes nothing, and the dashboard's 409 names the link cause. For an explicit Default that cannot clear the file itself:test/test_chat_slot_reasoning_effort_explicit_default.pychecks the value each handler path leaves, set on a stopped chat, a deferred pick, a raised clear and a model without effort, cleared by a level or a live clear, including on the binding a rebound push landed on, restored by every rollback unless a cold start during the reset already applied and cleared it, and not written again after such a start, while a save that fails refuses the pick before anything changes, or warns once the live session already runs it, a pick the new binding cannot save after a rebind is refused with the slot unchanged so a retry runs again, after the session its push reached is put back on the slot's value (a Default by the automatic clear), and is kept with a warning when that session cannot be put back, a level saves the Default it clears before its live push, is refused unpushed when that save fails, and puts the Default back when the request does not end on the level, a pick the session map refuses answers 503 without saving or changing the slot, a cancelled save puts the prior value back, and a pick that clears a pending Default holds its row until the request ends, so a failed save or a rolled-back reset puts the flag back inside that hold; a pick that would change the flag on a session whose cold start is in flight answers 409turn_in_flightand changes nothing, for a Default and for a level over a pending Default on the reset and deferred paths and for a level's clear before a live push, under either spelling of a folded key, and lands once the start registers, while a level pick with nothing to record is deferred during a live turn and a Default pick on a pending Default stays refused, while the request-exit put-back still restores a Default the pick cleared;test/test_session_pool_explicit_default.pyruns a realSessionManager, where a pending flag skips the warm pool, arms the provider, is still set in a fresh read of the session map while the start runs, and is cleared and saved only once the projection applied it, while a second same-key start that read the same pending Default waits for the first to register or fail and starts no runtime of its own; a manager dropped mid-start leaves it for the next manager, which applies it; it stays pending when the projection could not rewrite the file or the start failed or was cancelled before it ran, stays clear in memory when the clear's save fails or is cancelled, is armed by only one of two starts under the two spellings of one key, holds the key's allocation reservation from before it reads the flag until the session is registered, reads the flag only after a pick's save still in flight settles, under either spelling of a Slack thread's key and before either has a live session (the saved value, or the one a failed or cancelled save put back), does not arm a flag another start under the key cleared after this one read it, is left pending, neither armed nor cleared, by a start on a level its caller chose, the next start without one applies it, and it survives a reset, another chat's Default pick is refused while a start that did not apply the flag or failed leaves it armed, and the row is free again once an applied start clears it, a level start writes nothing to its key's flag and keeps the row for the Default it leaves pending, andLLMProviderdeclares both hooks with defaults under which a provider without the overlay clears it at once;test/test_effort.pychecks the projection removes an operator entry once and keeps the guard on the next spawn and forauto, leaves a refused file pending, and changes nothing for a model without effort or a backend that reads no overlay;test/test_session_map_mirror_flags.pychecks the flag survives a prune, that a new row past the 1,000-session bound or with a key past 200 characters is refused without writing while clearing or re-setting a flagged row never is, that a row whose flags are not an object does not break the count, reads as having no flag, and gets a fresh object when a flag is set or cleared on it, and that a held row stays counted until its last hold ends: a newcomer is refused, the held key arms into it at the full bound, a released hold frees it, a key with no row gets no hold, a hold never lifts the key bound, and the manager arms again the spelling its hold kept.test/test_dashboard_chat_handlers_coverage.pyandtest/test_switch_handlers_sibling_alias_busy.pygive their mocked session manager the save the effort pick awaits.test/link_screen_sites.pyrecords the reader's link check for the repo's link-screen gate.test/test_session_work_dir.pychecks a one-run directory holding a projection is still reclaimed.test/test_eager_spawn.pychecks that a Default pick cannot land between an eager prefetch's read of the slot's level and its allocation.test/test_session_control_set_model_effort.pychecks that asession_set_modellevel committed at the turn start clears a pending Default under both spellings of a folded key, also when the level is unchanged, that a model-only pick, a superseded effort half and a pick the gate refuses leave it pending, that the turn saves the clear before it acquires a session, and that a failed or cancelled save restores the pick, the slot's values and the flag while the flag's row stays held; that a Default picked on another slot of the session after the call, or a level there that clears a pending Default, supersedes its effort half, a level with no Default pending does not, every open slot on the session (under either spelling of a folded key) is bumped and no other, and a failed save in the effort picker leaves the call to apply;test/test_session_pool_explicit_default.pychecks that the next start after such a commit applies no Default and leavescli.jsonbyte for byte.test/test_session_control_set_model.pyfollows the turn-start call into its new helper.test/test_acp_tool_search.pycovers the Tool Search side,test/test_acp_provider.pythe warm-pool push, andtest/test_per_runtime_process_resources.pyimports the one_read_cli_overlaytest helper fromtest/test_effort.py.test/test_effort_spawn_fence.py::TestDefaultSpawnOverlayFenceholds a Default spawn inside its runtime start and checks that no level lands until it returns: a live pick on the same model, a pick on another model during anautospawn, a live explicit Default that writes a workspace default, and another session's level projection, whose own/effortpush still lands its level. It also checks that two Default spawns share the window, that a Default spawn arriving behind a queued write waits for it, that an explicit Default start with a workspace default takes the write side, that a level spawn releases the fence before its runtime start, that a Default spawn arriving behind a level write reads a cleared file, that sequential writes release the fence, that the fence follows the resolved work directory through a symlink, that a Default spawn waits out a pick still writing past the spawn ceiling, that a spawn waiting for its directory holds off nothing, so a write arriving then lands first while a later write waits for the spawn's runtime start, that an expired window fails its start before Tool Search or the runtime, that a window cancelled while it waits leaves nothing behind, that an expired pick raises the existing persist error while an expired clear returns no answer and changes nothing, and that a start cancelled once or twice leaves its window only after its overlay worker settles.test/test_effort_spawn_fence_order.pychecks the order with the cold-start queue: a start waiting for its directory holds no cold-start permit, a start queued for one holds no window, a write arriving while a start queues gets the permit handed back, the fence ceiling never counts the queue wait, the start's first spawn uses its permit without queuing again, a task started inside the window queues for its own, and a start that never spawns or whose fence wait expires returns its permit. It also checks that a start reports its admission grant once, before the projection, the Tool Search write and its runtime start, whether or not that runtime spawns.test/test_chat_slot_reasoning_effort.pychecks that a pick whose fence wait expires falls back to the reset with the slot value committed and that the busy answer names the fence, andtest/test_harness_parity.pythat a backend reading no overlay takes no fence. The fence keys a work directory by its real path, resolved in a worker thread before the registry lookup, so a slow or wedged workspace cannot stall the gateway loop; the lookup itself stays synchronous, because an await between its get and its create would give two chats two fences for one directory.test/test_effort_shared_overlay.py::TestEffortPathsDoNoFilesystemIoOnTheLoopasserts that property rather than a call site: it fails ifstart()for a Default or a level spawn,change_effort()or eitherclear_effort()branch makes a work-directory filesystem call on the loop thread, and it checks thatstart()still does that work on worker threads. The fence tests also check that a work directory reached through a symlink shares one fence with its real path, that two holders resolving the key at once share one fence, that two work directories sharing one admitted settings folder share one fence, that a refused settings link keys the fence on the work directory's own.kiro/settingsname, that a key the resolution cannot produce fails the start and leaves nothing held, and that a start cancelled while resolving leaves nothing held.test/test_session_map_mirror_flags.pyalso checks that only a storedtrueis a pending explicit Default: a stored"false","true",1,0,nullor{}is not pending, is not counted toward the row bound, and does not keep its row through a prune. A live push that fails while another holder keeps the settings lock past the 2 s launch wait still puts the prior level back in the file and in the session's map.Main's 2,000-line file cap: the PR's tests for
test/test_effort.py,test/test_chat_slot_reasoning_effort.py,test/test_session_map_mirror.pyandtest/test_session_pool.pylive in topic siblings (test_<stem>_<topic>.py) with shared helpers in<stem>_helpers.py, as #20340 did..github/black-baseline.txtdrops itstest/test_chat_slot_reasoning_effort.pyrow, since that file is black-clean once its new tests moved out. One file already over the cap and listed in its baseline grows here, which the guard reports without failing:src/kiro_crew/security_posture.py.src/kiro_crew/session_map.pyalso grows, and stays under the cap.test/test_acp_cli_overlay_bom.py, carried over from #16251: acli.jsonsaved with a UTF-8 byte-order mark keeps the user's settings through an effort write, a Tool Search write and an explicit clear, and is written back without the mark.The ownership stamp: an operator's save with the same level Kiro Crew recorded, including one that lands right after Kiro Crew's rename or one stored a kernel clock tick behind the publish on nanosecond or FAT two-second file times, keeps that entry through a Default or
autospawn's clear; Kiro Crew's own write keeps its record valid; a Tool Search write drops a void record instead of re-stamping it; and Code Review Sage keeps an earlierAcpProviderrecord valid.test/test_atomic_write_pinned_parent.pychecks the published file carries the requested modification time on both the by-name and descriptor paths, andtest_review_pool.pychecks Sage's overlay write runs off the event loop.Tests that need a platform primitive do not skip at run time. The symlink-mechanism tests carry the existing
requires_symlinksprobe marker fromtest/conftest.py, which GitHub's Windows runners pass, and the descriptor-pinned tests carry the existingrequires_o_nofollowprobe marker (O_NOFOLLOWis part of whatpinned_fs.supports_pinned_walk()requires). Hardlinks are required outright and directory aliases useplatform_compat.symlink_or_junction. The two synthetic-swap tests run everywhere: where Windows pins refuse the swap, they assert the lock still lands in the work directory and nothing appears outside it. Whereos.mkfifois missing, the FIFO case stubsos.fstatto report a FIFO forcli.json, which both readers check. The by-name floor the descriptor paths fall back to is tested on every platform.Committed-file stability: with a valid record, two same-level effort writes, two Tool Search writes, and a repeat skill projection or Code Review Sage write each leave
cli.jsonbyte-identical with its mtime unchanged; with no record, a repeat Tool Search write leaves it byte-identical and a Tool Search write stores nokirocrew.*key; a level change still writes and restamps, and an outside rewrite still voids the record.Deep nesting: a valid
cli.jsonnested deeper than the interpreter can copy leaves a Default orautostart, the clear and skill projection running, with the file byte-for-byte unchanged and its mtime kept.test/test_unpinned_effort_binding.py(main's #18905): its construction test now asserts that nothing is adopted from the file, and new tests pin, for a chat with nothing pinned, the restart'sautosweep under the window fence (fromautoand from the""the session provider records after a first start), the post-spawn push of the bound level, and the bind on a fresh warm-pool conversation that serves another default model.Manual verification
Before the fix, on a live gateway: the shared workspace overlay held Max for three models, left by three older chats, and a chat whose picker showed Default ran at Max.
kiro-cli 2.24.1 with this branch's file shape: a work directory whose
cli.jsonholdschat.modelDefaults(claude-opus-4.7low),toolSearch.enabledandkirocrew.effortOwned.kiro-cli settings listreports the effort and Tool Search entries as loaded from the workspace.kiro-cli chat --no-interactivewith a minimal local agent starts and answers normally, the same as a control directory without the key, andkiro-cli settings --workspacewrites keep the key. The tests construct the real provider against a real work directory, which is where the leak happened.Live on kiro-cli 2.24.1 through this branch's
AcpProvider, reading the effort kiro-cli itself reports in its_kiro.dev/metadatanotification (reasoning.effort), not Kiro Crew's own state. A chat that pickedhighranhigh. A second chat on Default in the same work directory then emptied that chat'scli.jsonentry and ran the model default, while the first chat kepthigh, andnew_conversation()came back onhigh. With an operatorlowin the file, a chat that pickedmaxranmaxand the file stayedlow. The same run with the post-spawn/effortpush switched off ranlow, so the push is what lands the pick. Not run: a full gateway and the real warm pool; a secondAcpProvideron Default stood in for the pool's spawn.kiro-cli 2.27.1, the version main bundles now, with this branch's writers producing each
cli.json:kiro-cli settings listloads the workspacechat.modelDefaultsand Tool Search entries with bothkirocrew.*keys present, and a chat answers as it does in a control directory. After akiro-cli settings --workspacewrite the file's mtime no longer matches the stamp, so the record reads as void. Over ACP, an/effort highpush returns "Effort set to high" and the next_kiro.dev/metadatareportshighwhere the file alone gavelow; sent before the first turn, as the branch sends it after a spawn, that turn answers athigh. On kiro-cli 2.29.1, the host's version now, a spawn with both keys in the file starts normally and runs the file's level.Release note
For the changelog when this ships:
.kiro/settings/cli.jsonstay after the upgrade, because they cannot be told apart from levels an operator wrote. A chat on Default in that project keeps running them, and each Default start logs a gateway warning that names them. To clear one, pick any level on that chat and then pick Default, or delete the entry fromchat.modelDefaultsby hand.kiro-cli settings --workspacewrite to that file voids Kiro Crew's ownership record. From then on the levels Kiro Crew wrote count as hand-written, and a chat on Default runs them again. The same warning names them, and the same steps clear them.Related Issues
Fixes #16893
Fixes #22821
Fixes #23396
#21376 fixed the KAS side of the same leak; this PR also stops kiro-cli reading the file back, so it replaces that PR's kiro-cli read-back test in
test/test_effort_overlay_readers.py. No open issue covers the kiro-cli leak itself. #11451 concerns the same file dirtying a from-source checkout and is not fixed here.Follow-ups, not fixed here: #19867 (opt-in to follow a
.kiroshared across worktrees), #14840 (move live effort off the deprecated/effortslash command) and #22496 (carry a session's effort per session instead of through the shared work-directorycli.json).Pattern harvest
Rule candidate: review-prompt
Pattern: state kept in a file shared by several sessions (keyed by directory) is read back as one session's own setting.
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)