Skip to content

fix(acp): keep a chat's effort pick from leaking into other chats - #14528

Open
unstablebrainiac wants to merge 1 commit into
kirodotdev:mainfrom
unstablebrainiac:fix/effort-overlay-leak
Open

unstablebrainiac wants to merge 1 commit into
kirodotdev:mainfrom
unstablebrainiac:fix/effort-overlay-leak

Conversation

@unstablebrainiac

@unstablebrainiac unstablebrainiac commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

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 .kiro or settings link out of the work directory. Where .kiro links 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

  • Changing the effort picker or the levels. The overlay keeps its format; it gains only two Kiro Crew keys, kirocrew.effortOwned and kirocrew.effortOwnedStamp, which kiro-cli loads without complaint (Manual verification).
  • Making the overlay per session. Its unit stays the work directory; this PR only stops it being read back as a session's own choice.
  • Dropping the session's level from cli.json and relying on the /effort push 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's low after session/set_model (away and back, or to the current model) and after session/set_mode. With no entry in the file, the next request after either switch carried no effort at all. /clear, /compact, session/load and rewrites of the file kept the pushed level.
  • Carrying a sub-agent's spawn-time effort into 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)

Case Before After
The overlay names a level Kiro Crew wrote for this model, and this session did not pick it adopted as this session's own pick removed before the spawn, so the model runs at its default; one info line names the file and the removed level
The overlay names a level the operator wrote by hand adopted left in the file and not adopted; kiro-cli applies it as the operator's own setting
This session picked a level other than the one the operator wrote the operator's level was overwritten, then deleted by a later clear the operator's level stays; the pick reaches this session through /effort
The session model is auto kept every level, so kiro-cli applied one to whichever model it chose every level Kiro Crew wrote is removed; hand-written ones stay
A Default or auto spawn may run a level Kiro Crew did not write silent one warning names the model, the level and the cli.json path
This session has its own level written before the spawn written before the spawn unless a value Kiro Crew did not write is there, then pushed with /effort after the spawn and after each fresh warm-pool conversation
Another session removes this session's level between its write and the spawn read ran the default while the slot showed the level runs its level, because of the /effort push
A KAS session, which reads no cli.json and takes its level as the effortLevel config option (main's #18630) construction read the file's level for its model, so a Default chat could push another chat's pick (main's #21376 stopped KAS reading and writing the file) starts on its own level, pushed as effortLevel, and writes no overlay; the post-spawn /effort push skips it, since KAS answers that command with -32603
A chat with no model pinned, whose level main's #18905 binds to the model the backend reports serving a restart projected the level for the model served last, so a spawn on another default model could read another chat's level, and a fresh warm-pool session on another default model got none every spawn with nothing pinned removes the levels Kiro Crew wrote, as an auto spawn does; the chat's own level is bound to the served model and pushed with /effort after each spawn and each fresh warm-pool conversation
Another chat is writing the overlay as this session starts the lock gave up at once waits up to the 2 s startup ceiling
An effort under the other model family's key, which kiro-cli ignores removed by the next write left alone
A live model switch on a Default slot removed the new model's hand-written level leaves it; only choosing Default in the picker removes an entry Kiro Crew did not write
Default is picked on a stopped chat, during a turn, or where the live clear raises the next spawn kept a level Kiro Crew did not write and ran it under Default a one-shot explicit_effort_default flag 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 409 turn_in_flight, and past 1,000 flagged sessions a pick answers 503, both before anything changes; its lifecycle is the last row
A session_set_model level is committed at the turn start while a Default is pending (main's #15422) n/a the commit clears the pending flag in the same synchronous step, and the turn saves the clear before it acquires a session, so a later start without its own level does not remove an entry written after the newer level; a failed or cancelled save puts the pick, the slot's values and the flag back, and the pick applies on the next turn; a pick in the effort picker on any slot of that session after the call that changes its pending Default (a Default, or a level that clears one) supersedes the call's level, while a level with no Default pending does not
cli.json is larger than 1 MiB read in full at each chat start, on the event loop: 4.3 s and 997 MiB at 256 MiB (#23396) not read: a write raises and leaves it unchanged, a clear fails, a Default or auto spawn 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 write
A Kiro Crew rewrite of a cli.json holding non-ASCII text, such as CJK every writer escaped it, roughly doubling CJK text; main reads any size all four writers keep it as UTF-8, escaped only where it cannot be encoded (an unpaired surrogate), so a rewrite cannot double it past the 1 MiB ceiling; an effort write whose result would pass 1 MiB is refused with the file unchanged, and a clear refused that way fails. The skill projection and Code Review Sage writers do not refuse; a file one of them re-indents past 1 MiB is the row above
A valid cli.json too deep for copy.deepcopy but not for json.loads rewritten, unless json.dumps raised the effort, Tool Search, clear and Code Review Sage writes copy what they read to skip unchanged rewrites, so they leave it unchanged and a clear fails. Skill projection copies without recursion and still projects; past json.dumps's depth it leaves the file, logs once and uses authored agents
cli.json is a link, a hardlink or not a regular file, or a .kiro or settings link names a folder outside the work directory followed: the linked file was read and merged into the workspace file, and a linked folder took the write wherever it pointed refused the same way, judged from the link's own text without opening its target; a link to a folder inside the work directory is followed, and every write goes through the settings folder opened without following links
A .kiro or settings link names a folder inside the work directory that is on the sensitive-path list, such as ~/.aws when the work directory is the home directory followed, read and written refused before any lock, read or write: a write raises and the folder is left unchanged, a clear fails, a Default or auto spawn logs that it may run another session's level
Another writer (kiro-cli's own settings --workspace, a hand edit, a second gateway) changes cli.json after Kiro Crew read it and before it publishes overwritten with the copy Kiro Crew read left as that writer left it: a write raises and a clear fails, also when the write would have changed nothing; a writer that skips the lock can still land between the last check and the rename
No overlay in the directory untouched untouched: no file, no lock
The one-shot explicit_effort_default flag of the explicit Default row, in full n/a a one-shot explicit_effort_default flag 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, or auto, 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 with session/load keeps 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 409 turn_in_flight and 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 registered

A chat on Default effort ran another chat's level because AcpProvider.__init__ read every chat.modelDefaults entry 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_overlay before every spawn. The same file holds kiro-cli's operator-editable workspace settings, so Kiro Crew only changes what it wrote itself. _write_cli_overlay records its {model: level} in a flat kirocrew.effortOwned key inside the same cli.json, so the level and its record land in one atomic write. Each write also stores kirocrew.effortOwnedStamp and sets it as the staged file's modification time before the rename (mtime_ns in atomic_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 JSON null or 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 a chat.modelDefaults that is not an object as it is. Only the dashboard's explicit Default pick, in dashboard/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_effort pushes it with the same /effort command the picker uses. Before a Default or auto spawn, 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): a cli.json that 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_settings in workspace_cli_settings.py opens the settings folder without following links and takes the lock there; the read and _publish_cli_overlay use that folder, and the write raises if .kiro/settings then 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, so start() runs the projection through asyncio.to_thread, and a cancelled start() 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 in providers/acp.py and docs/system-specs/modules/providers.md describe 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/new and 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 own settings --workspace write, 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 or auto spawn warns about them. To clear one, pick a level in the effort picker and then pick Default (on an auto chat, select the listed model first), or delete the entry from cli.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 /effort push, so a failed push leaves that session at the operator's level; the failure is logged. kiro-cli 2.27.1 lists /effort as deprecated in favour of its /model settings 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 of cli.json, including kiro-cli's own settings --workspace write, voids the ownership record, so Kiro Crew keeps its own levels after it and the Default or auto spawn's warning names them. The ownership record depends on kiro-cli loading a workspace cli.json that 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.py fails if the bundled kiro-cli pinned in packaging/kiro-cli-version is 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 or auto chat can run a stale level, and that chat's pre-spawn warning names it. docs/reference/kiro-cli/README.md names the checks to re-run after each kiro-cli update: kiro-cli still loads such a file, its own settings --workspace write moves the mtime off the stamp, and a live /effort push 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, and settings is 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 create settings or 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 in session/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 409 turn_in_flight while 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 from cli.json has already run, and the start that ran it cleared the flag, so it does not run again.

Backwards compatibility

Breaking: a cli.json that is a link or a hardlink is refused instead of followed, and so is a .kiro or settings link 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 or auto spawn 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 on c24c7bc8d). A .kiro shared 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 a cli.json whose chat.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 409 effort_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 409 turn_in_flight a 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's cli.json are preserved, including an effort under the other model family's key. Upgrading: a level an older build wrote has no stamped kirocrew.effortOwned record, 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.effortOwned and kirocrew.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 committed cli.json byte-identical, as on main.

Also taken away: an effort or Tool Search write that finds cli.json unparseable 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_config or reasoning), 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 leaves cli.json untouched.

Readers of what this PR takes away, found by grepping origin/main at 99c76a43d; a test pins each:

  • Reader: src/kiro_crew/acp/skill_projection_parts/preparation.py:prepare_native_skill_projection -- chat -- a refused settings lock skips the projection with one logged line and the session starts on its authored agents, and a cli.json nested past Python's recursion limit is left unchanged the same way (the RecursionError is 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_projection
  • Reader: src/kiro_crew/apps/builtins/code_review_sage/sage_lib/review_pool.py:_write_effort_overlay -- app -- catches the lock's refusal and logs, so the review runs without its effort overlay, and the write runs through asyncio.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_write
  • Reader: src/kiro_crew/session_work_dir.py:CLI_SETTINGS_LOCK_NAME -- chat -- the lock file name is unchanged and a run directory holding only a projection is still reclaimed; test/test_session_work_dir.py::TestReclaimRule::test_a_directory_holding_an_effort_projection_is_removed
  • Reader: src/kiro_crew/providers/acp_cli_overlay.py:_apply_tool_search_overlay (bound as AcpProvider._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_path
  • Reader: website/src/lib/slotSwitch.ts:performSlotSwitch -- chat -- every effort pick (picker, chat pane, model menu, keyboard cycle) goes through it, and a refused answer (409 turn_in_flight, 409 effort_overlay_busy, 503 effort_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)"
  • Reader: website/src/utils/agentSwitchFeedback.ts:agentSwitchFailureMessage -- chat -- shows the retry-later copy for 409 turn_in_flight; website/src/test/agentSwitchFeedback.test.ts "maps a 409 turn_in_flight to the specific retry-later copy"
  • Reader: src/kiro_crew/providers/acp.py:AcpProvider.init -- chat -- construction no longer reads chat.modelDefaults back from the overlay, so a session starts on its own level: a dashboard slot's saved level, ConfigLoader.resolve_session_effort for 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_removed
  • Reader: src/kiro_crew/providers/acp_model_effort.py:clear_effort (bound as AcpProvider.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_default

Main's #21009 split session.py into owner modules, so the flag's manager methods live in session_parts/turn_context.py, the in-flight intent-write record in session_parts/record.py, and the warm-pool fence delegators in session_parts/runtimes.py; session.py only binds them.

Tests

test/test_effort.py and its siblings test/test_effort_*.py build real kiro AcpProviders on temp work dirs, the path the leak went through. Tests that fail on main:

  • a level Kiro Crew left for the same model is not adopted, and is gone before the first spawn (on main: assert 'max' is None fails);
  • a level for another model is neither adopted nor removed;
  • construction leaves cli.json untouched, and a later spawn projects again and removes a level written before it;
  • an auto session removes the levels Kiro Crew wrote for every model;
  • a spawn waits out another thread holding the settings lock for 0.1 s, then clears the leftover level;
  • a peer's clear during the spawn (an auto peer and a same-model Default peer) does not change what the session runs: /effort max is 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_unsaved with 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 a cli.json whose chat.modelDefaults, model entry or active effort key is not an object refuses, leaves the file byte-for-byte unchanged and answers 409 effort_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 an auto session; 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 malformed cli.json is left unchanged; an auto session keeps a chat.modelDefaults that 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 JSON null survives a projection and an ownership-guarded clear byte for byte, and a record entry of null or "" 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 an auto session creates .kiro/ in an empty directory; a Default session with no workspace default pushes nothing; a rejected /effort push does not break the start; a clear that loses the lock returns False and leaves the file; a cancelled start() 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 inside try/finally, so a failed assertion still releases and joins it. For the kept-value design: an operator's low survives a chat that projects max, and a later automatic clear keeps it; a stale record over a changed value is dropped without touching the value; a present non-object chat.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 a chat.modelDefaults that 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 or auto; a Default or auto spawn 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. A cli.json holding an integer past Python's 4,300-digit limit, or nesting past the recursion limit, reads as unparseable: a clear, a Default or auto projection and start() 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.py lists providers/acp.py among the modules whose redaction feeds a local log line rather than an egress path, which the redaction call-site check in test/test_security_posture.py requires 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. A cli.json past 1 MiB is left byte-for-byte unchanged: an effort write raises, a clear returns False and logs the ceiling, and an auto spawn logs that it may run another session's level. Over the same file, change_effort raises and the explicit Default returns None, each keeping the session's level and naming the ceiling, and the dashboard's 409 effort_overlay_busy names the 1 MiB limit. test/test_kiro_cli_effort_ownership_pin.py fails when packaging/kiro-cli-version pins a kiro-cli newer than 2.27.1. A cli.json that links to a file inside or outside the settings folder, dangles, or is a hardlink, and a .kiro or settings link 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/settings link to .aws in 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 .aws keeps its bytes and gains no lock or file; the pinned reader refuses a sensitive cli.json descriptor before reading it. test_settings_link_outside_is_never_probed_by_any_prelock_path records every lstat, stat, open, readlink, scandir, listdir and realpath across the effort, Tool Search, clear, Default, fence-key and skill-projection paths with .kiro or settings linked outside, and finds none at or below the outside folder. test/test_native_skill_projection_pinned_settings.py replaces 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 a cli.json that 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, a cli.json that changes during every read skips the projection the same way. The fence, fence-order, shared-overlay and Tool Search test files request no_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 over cli.json right 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 its cli.json bytes and gains no sibling, and the moved folder is not written. The same holds for a folder replaced before the read, whose cli.json is never read, and for one replaced after the write's own check. A .kiro or settings folder 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 no settings created in the linked folder, a fresh work dir still gets .kiro/settings with 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 .kiro cannot 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 .kiro and settings stay 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's cli.json keeps its bytes; Windows runs both with a junction. A reparse point taken in place by the held .kiro folder is refused before settings is created in it, or once it is, and one taken by the held settings folder is refused before the lock file opens and after it; both are simulated through the stat attribute. Over a linked cli.json, change_effort raises 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.py checks 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 409 turn_in_flight and 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.py runs a real SessionManager, 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, and LLMProvider declares both hooks with defaults under which a provider without the overlay clears it at once; test/test_effort.py checks the projection removes an operator entry once and keeps the guard on the next spawn and for auto, 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.py checks 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.py and test/test_switch_handlers_sibling_alias_busy.py give their mocked session manager the save the effort pick awaits. test/link_screen_sites.py records the reader's link check for the repo's link-screen gate. test/test_session_work_dir.py checks a one-run directory holding a projection is still reclaimed. test/test_eager_spawn.py checks 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.py checks that a session_set_model level 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.py checks that the next start after such a commit applies no Default and leaves cli.json byte for byte. test/test_session_control_set_model.py follows the turn-start call into its new helper. test/test_acp_tool_search.py covers the Tool Search side, test/test_acp_provider.py the warm-pool push, and test/test_per_runtime_process_resources.py imports the one _read_cli_overlay test helper from test/test_effort.py. test/test_effort_spawn_fence.py::TestDefaultSpawnOverlayFence holds 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 an auto spawn, a live explicit Default that writes a workspace default, and another session's level projection, whose own /effort push 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.py checks 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.py checks 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, and test/test_harness_parity.py that 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::TestEffortPathsDoNoFilesystemIoOnTheLoop asserts that property rather than a call site: it fails if start() for a Default or a level spawn, change_effort() or either clear_effort() branch makes a work-directory filesystem call on the loop thread, and it checks that start() 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/settings name, 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.py also checks that only a stored true is a pending explicit Default: a stored "false", "true", 1, 0, null or {} 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.py and test/test_session_pool.py live in topic siblings (test_<stem>_<topic>.py) with shared helpers in <stem>_helpers.py, as #20340 did. .github/black-baseline.txt drops its test/test_chat_slot_reasoning_effort.py row, 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.py also grows, and stays under the cap.

test/test_acp_cli_overlay_bom.py, carried over from #16251: a cli.json saved 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 auto spawn'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 earlier AcpProvider record valid. test/test_atomic_write_pinned_parent.py checks the published file carries the requested modification time on both the by-name and descriptor paths, and test_review_pool.py checks 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_symlinks probe marker from test/conftest.py, which GitHub's Windows runners pass, and the descriptor-pinned tests carry the existing requires_o_nofollow probe marker (O_NOFOLLOW is part of what pinned_fs.supports_pinned_walk() requires). Hardlinks are required outright and directory aliases use platform_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. Where os.mkfifo is missing, the FIFO case stubs os.fstat to report a FIFO for cli.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.json byte-identical with its mtime unchanged; with no record, a repeat Tool Search write leaves it byte-identical and a Tool Search write stores no kirocrew.* key; a level change still writes and restamps, and an outside rewrite still voids the record.

Deep nesting: a valid cli.json nested deeper than the interpreter can copy leaves a Default or auto start, 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's auto sweep under the window fence (from auto and 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.json holds chat.modelDefaults (claude-opus-4.7 low), toolSearch.enabled and kirocrew.effortOwned. kiro-cli settings list reports the effort and Tool Search entries as loaded from the workspace. kiro-cli chat --no-interactive with a minimal local agent starts and answers normally, the same as a control directory without the key, and kiro-cli settings --workspace writes 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/metadata notification (reasoning.effort), not Kiro Crew's own state. A chat that picked high ran high. A second chat on Default in the same work directory then emptied that chat's cli.json entry and ran the model default, while the first chat kept high, and new_conversation() came back on high. With an operator low in the file, a chat that picked max ran max and the file stayed low. The same run with the post-spawn /effort push switched off ran low, so the push is what lands the pick. Not run: a full gateway and the real warm pool; a second AcpProvider on 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 list loads the workspace chat.modelDefaults and Tool Search entries with both kirocrew.* keys present, and a chat answers as it does in a control directory. After a kiro-cli settings --workspace write the file's mtime no longer matches the stamp, so the record reads as void. Over ACP, an /effort high push returns "Effort set to high" and the next _kiro.dev/metadata reports high where the file alone gave low; sent before the first turn, as the branch sends it after a spawn, that turn answers at high. 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:

  • Effort levels an older Kiro Crew wrote to a project's .kiro/settings/cli.json stay 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 from chat.modelDefaults by hand.
  • A kiro-cli settings --workspace write 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 .kiro shared across worktrees), #14840 (move live effort off the deprecated /effort slash command) and #22496 (carry a session's effort per session instead of through the shared work-directory cli.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

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@unstablebrainiac
unstablebrainiac requested a review from a team as a code owner September 27, 2026 20:29
@github-actions github-actions Bot added the fork Pull request from a fork (external contributor) label Sep 27, 2026
@unstablebrainiac

This comment has been minimized.

@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 27, 2026
@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of 2dfd5d911c2a652eb87cc7e8266dc7d353811662 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

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 /effort fits that constraint.

Notes

  • The case the PR was written for, three Max levels left by older chats, is not fixed by upgrading. A level written by an older build carries no ownership record, so _warn_unowned_cli_effort treats it as "a level an older build projected without a record": it stays in the file, and a Default chat keeps running Max with only a log line. Say this in the description, or the Goal reads as met when it isn't for existing work directories.
  • The fix rests on /effort, which kiro-cli 2.27.1 marks deprecated, and on matching a whole-second mtime stamp. The tripwire test covers only the bundled kiro-cli pin. On a host-installed kiro-cli that drops /effort, every chat with its own level would quietly run another chat's level. The fallback should be a warning in the log, not silence.
  • Several fixes ride along with the effort change: the 1 MiB cli.json cap (A kiro chat start reads the workspace cli.json in full on the event loop, at any size #23396), the UTF-8 writers, the deepcopy-depth handling and the .kiro link refusal (Effort writes follow a .kiro link out of the work directory into the global kiro-cli settings #22821). They are about 920 KB of the diff, and as separate PRs each could be reviewed and reverted on its own.

[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 .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5, fork) — ✅ PASS

Premise-level review of 2dfd5d911c2a652eb87cc7e8266dc7d353811662 via the fork AI-review pipeline — 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 does not.

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

  • Item 8 — rides along, oversized: the 1 MiB cli.json ceiling is for A kiro chat start reads the workspace cli.json in full on the event loop, at any size #23396, not for this PR's Goal ("A chat's reasoning-effort pick applies to that chat only"). The base workspace_cli_settings.py has no size ceiling (grep MiB|_MAX_.*BYTES|1024 \* 1024: 0 hits). The ceiling then forces item 9 (all four writers moving to ensure_ascii=False "so a rewrite cannot double it past the 1 MiB ceiling"). The copy.deepcopy comparisons and RecursionError handling (item 10) were added in the same PR. Subtraction: defer the ceiling and the UTF-8 writer change to their own PR against A kiro chat start reads the workspace cli.json in full on the event loop, at any size #23396. That PR then becomes the smaller effort-ownership fix plus a separate size fix.
  • Item 5 — inherited: the ColdStartPermit ContextVar handoff exists only because "twenty of the provider's test files stub" _start_kiro_runtime. The reason it gives is test seams, not a user harm.

What this change ships

Inventory (10 items) — 7 justified

Intent: 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.

  1. A Default chat no longer runs a level another chat wrote; levels Kiro Crew wrote are removed before spawn — justified
  2. Hand-written cli.json levels are kept and named in a warning — justified
  3. A pinned chat's own level is pushed with /effort after spawn and after each warm-pool conversation — justified
  4. A Default picked on a stopped chat is carried as a one-shot flag; the next start skips the warm pool; picks are refused with 409/503 at the edges — justified
  5. A start's cold-start permit is handed to its spawn, and a work-dir spawn fence holds level writes off — justified
  6. A .kiro/settings link leaving the work dir is refused (Effort writes follow a .kiro link out of the work directory into the global kiro-cli settings #22821) — justified
  7. KAS starts on its own level and writes no overlay — justified
  8. cli.json over 1 MiB is no longer read; writes and clears fail, and the picker reports the limit — rides along (A kiro chat start reads the workspace cli.json in full on the event loop, at any size #23396)
  9. All four writers keep non-ASCII text as UTF-8 — rides along, follows from item 8
  10. Very deep cli.json documents are left unchanged by the effort, Tool Search and Sage writers — rides along, undeclared purpose

[FIRST-PRINCIPLES-REVIEWED] 2dfd5d9

Description read

[DESCRIPTION-READ] cceb45936028e86bb69f308ccf07946114c642c2c4d8d34f345360a1673c2979

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review (fork) — ✅ no blocking findings

Reviewed 2dfd5d911c2a652eb87cc7e8266dc7d353811662 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2dfd5d9

@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review (fork) — ✅ no blocking findings

Reviewed 2dfd5d911c2a652eb87cc7e8266dc7d353811662 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 2dfd5d9

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

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

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@unstablebrainiac

This comment has been minimized.

@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 28, 2026
@iamwhatever

Copy link
Copy Markdown
Contributor

HOLD at c640ad3.

This head is a rebase of a777826 plus one test-import change in test/test_session.py. Production code is unchanged since r116, and Opus has no findings.

The new lane run on this head raised items that have no reply yet:

  1. GPT 6.1 (acp_model_effort.py#L854), downgraded but undisposed: when the 1 MiB guard refuses the overlay write for an unpinned chat set to High, no /effort is sent and the chat runs the default while the picker shows High. Please post a target=gpt head=c640ad3 disposition (fix, rebut or linked issue).
  2. Design notes: after an upgrade, levels an older build wrote have no ownership record, so Default chats keep running Max (the motivating case) with only a log warning as the remedy. The mtime stamp also drops ownership on any outside rewrite of cli.json. Please say whether the PR accepts these, or adopt record-less entries once on upgrade.

The blockers from r116 still stand, and only a maintainer can clear them. The PR adds a NON_EGRESS_REDACTION_MODULES row and settings-path containment, so it needs human security review. Sensitive Change Review is red and a fork cannot pass it. First Principles items 3, 6 and 8 still need a scope ruling. #22496 tracks the per-session root fix. Our earlier request for changes stays until these clear. I did not re-read the full diff or run tests.

— crew-pr-reviewer (r117)

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author

@iamwhatever

  1. GPT on c640ad3, acp_model_effort.py#L854: linked issue Effort picked on a chat with no model pinned is not applied when cli.json is over 1 MiB #22819. The finding holds for a chat with no model pinned. That chat's first level push runs through change_effort, whose overlay write the 1 MiB guard refuses, so the level is logged and not applied while the picker shows it. A pinned chat keeps its level through the same refusal and gets it by /effort after the spawn. The description's 1 MiB row states both cases. It is not fixed here: it needs a workspace cli.json over 1 MiB, it is logged, and the lane's adjudication rated the remedy out of proportion. Effort picked on a chat with no model pinned is not applied when cli.json is over 1 MiB #22819 describes the fix and its tests.

  2. The PR accepts both Design notes, and the Release note says so. A level with no ownership record cannot be told apart from one an operator set with kiro-cli settings --workspace or by hand, so adopting record-less entries on upgrade would let the next Default start delete the operator's own setting. The log warning is not the only remedy. The Release note's first step, picking a level on that chat and then Default, removes such a level whoever wrote it while the chat runs, because that Default is the one explicit operator clear. The second step is deleting the entry by hand. The mtime stamp voids the record on an outside rewrite on purpose. The record holds only a value, so an operator who rewrites the same level looks like Kiro Crew's own write, and the moved mtime is what makes Kiro Crew keep that level instead of deleting it.

  3. The other items are steps only a maintainer can take: the security review, carrying the PR past Sensitive Change Review, and the scope ruling on First Principles items 3, 6 and 8.

@iamwhatever

Copy link
Copy Markdown
Contributor

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 cli.json rewrites keep non-ASCII text as UTF-8 through settings_json_text (workspace_cli_settings.py#L618-L632). I found no defect in that change. #22496, #22819 and #22821 track the deferred items.

What blocks approval:

  1. Human security review. The PR adds a NON_EGRESS_REDACTION_MODULES row (security_posture.py#L2221-L2225), settings-path containment (workspace_cli_settings.py#L86-L260) and stamping on the pinned-parent write.
  2. Sensitive Change Review is red, and a fork cannot pass it. A maintainer has to carry the PR.
  3. The current-head lane notes have no reply. First Principles item 9 calls the UTF-8 rewrite and the nested-file skip undeclared ride-alongs. First Principles item 3 (the mtime stamp) and the Design split note still need a maintainer's scope ruling. Design says the description was cut off before Backwards compatibility. The section does exist, but at about 54 KB the description is longer than the lane read, so it never checked the reader list. Shortening the description would fix that. Please post head=dd83aac dispositions and declare item 9 in the description.

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)

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author

@iamwhatever

  1. First Principles item 9. Neither part rides along; both follow from code this PR adds.

    • UTF-8: this PR adds the 1 MiB read ceiling, and the escaped rewrite roughly doubles CJK text. A file under the ceiling could be rewritten past it, and every later effort read would then refuse the file Kiro Crew had just written. That was GPT 6.1's upheld blocking finding on 3d767b9.
    • Deep nesting: the effort and Tool Search writes and the clear keep a copy.deepcopy of the document they read, so an unchanged document is not rewritten and its ownership record stays valid. That copy raises RecursionError on documents json.loads accepts, so those paths leave such a file as it is. The skill projection catches the same error and does the same.

    The What changed table now states both, inside the 8,000 bytes the lanes read.

  2. Design's description note. pr-description-capture.sh cuts what the lanes read at 8,000 bytes, and the explicit-Default row took 2,040 of them, which pushed every later row past the cut. That row now keeps a summary, and its full lifecycle text moved unchanged to the table's last row, so the 1 MiB, UTF-8 and deep-nesting rows fall inside the capture. The reader list does not fit in 8,000 bytes and stays under Backwards compatibility. It lists the refused .kiro or settings link as Breaking, with its skill projection, Code Review Sage and Tool Search readers.

  3. First Principles item 3 and the Design split note. As in the r117 reply, the stamp is what keeps an operator's own rewrite to the same level: value matching alone would take it for Kiro Crew's write and delete it. The scope ruling on both stays with a maintainer.

  4. The rulings for dd83aac are the ones above, in prose. No fork lane reads ai-review-disposition records; on main the only review lane that does is the same-repo codex-review.yml.

  5. Items 1 and 2 are steps only a maintainer can take: the security review, and carrying the PR past Sensitive Change Review.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on the mtime stamp every cli.json writer must carry: kept

The ownership record holds only a value, and an operator who rewrites the same level with kiro-cli settings --workspace or by hand writes the same bytes Kiro Crew wrote. That rewrite moves the mtime, which voids the record, so Kiro Crew keeps the operator's level instead of deleting it at the next Default start.
Every Kiro Crew writer of cli.json in this PR carries the stamp: the effort and Tool Search writes through _publish_cli_overlay, skill projection through carry_effort_ownership, and Code Review Sage through stamp_effort_ownership. A writer that dropped it would void the record, which keeps the level and logs a warning; nothing is deleted.
A record in Kiro Crew's data home would still have to recognise the file version it was written for, because the bytes alone cannot, so every Kiro Crew writer would have to update it after each rewrite: the same burden in a second place.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on the description cut before Backwards compatibility: addressed

The lanes read the first 8,000 bytes of the description (pr-description-capture.sh). The explicit-Default row of the What changed table took 2,040 of them. It now keeps a summary, and its full text moved unchanged to the table's last row, so the 1 MiB, UTF-8 and deep-nesting rows fall inside that read.
## Backwards compatibility does not fit with them. Its Breaking: line names the refused .kiro or settings link: the effort and Tool Search writes raise and are logged, and the picker reports that it could not change the file. Its Reader: lines give the three writers a linked-folder test each: skill projection with test_kiro_link_leaving_the_work_dir_skips_projection, Code Review Sage with test_a_settings_link_leaving_the_work_dir_gets_no_write, and Tool Search with test_settings_link_outside_is_never_probed_by_any_prelock_path. Each refusal is logged, and the session or review continues.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on the link refusal and Sage's off-loop write as separate commits: kept

The PR owner chose to keep the PR whole (reply 6138742562); the scope ruling is a maintainer's.
The link refusal fixes #22821: on main a .kiro -> ~/.kiro link sends a pick into the user's global settings, and this PR's ownership record, its stamp and the explicit Default carried to the next start would act there too. Sage's write moved off the event loop because it now waits on the settings lock every writer shares.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • First Principles item 3, the mtime stamp: rebutted

Value matching cannot tell an operator's rewrite to the same level from Kiro Crew's own write: both are the same bytes under chat.modelDefaults, so the next Default start would delete the operator's level. The rewrite moves the mtime, which voids the record, so Kiro Crew keeps that level.
Every Kiro Crew writer of cli.json in this PR carries the stamp: the effort and Tool Search writes through _publish_cli_overlay, skill projection through carry_effort_ownership, and Code Review Sage through stamp_effort_ownership. A writer that dropped it would void the record, which keeps the level and logs a warning; nothing is deleted.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • First Principles item 1, levels an older build wrote: rebutted

A level with no ownership record cannot be told apart from one an operator set for kiro-cli in the same work directory, so removing it on every Default start would delete the operator's own setting.
An explicit Default still removes it for a model that takes effort. Each Default start logs a warning naming the level, and the Release note and Known limitation give the steps: pick a level on that chat, then Default, or delete the entry.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • First Principles item 8, Sage's overlay write off the event loop: rebutted

Sage's overlay write now takes the cross-process settings lock every writer shares. Waiting on that lock on the event loop would stall every session in the gateway, so the write runs through asyncio.to_thread; Backwards compatibility lists it under _write_effort_overlay with test_write_effort_overlay_never_raises.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • First Principles item 9, the UTF-8 rewrite and the deep-nesting skip: declared

UTF-8: this PR adds the 1 MiB read ceiling, and the escaped rewrite roughly doubles CJK text, so a file under the ceiling could be rewritten past it and every later effort read would refuse the file Kiro Crew had just written. That was CI GPT 6.1's upheld blocking finding on 3d767b9.
Deep nesting: the effort, Tool Search, clear and Code Review Sage writes keep a copy.deepcopy of the document they read, so an unchanged document is not rewritten and its ownership record stays valid. That copy raises RecursionError on documents json.loads accepts, so those writes leave the file unchanged; skill projection copies without recursion and skips only a file json.dumps cannot serialize.
The What changed table now has a row for each, inside the 8,000 bytes the lanes read.

@iamwhatever

Copy link
Copy Markdown
Contributor

HOLD at dd83aac, reserved for human security review.

The head is unchanged since r120. Every current-head lane note now has a head=dd83aac disposition. First Principles item 9 is declared, and the What changed table states the UTF-8 rewrite and the deep-nesting skip inside the 8,000 bytes the lanes read (reply). The Design note on the description cut is addressed. I accept these replies.

What still blocks approval:

  1. Human security review. The PR adds a NON_EGRESS_REDACTION_MODULES row (security_posture.py#L2221-L2225) and settings-path containment (workspace_cli_settings.py#L86-L260).
  2. Sensitive Change Review is red on this head at its human-approval step, and a fork cannot pass it. A maintainer has to carry the PR.
  3. A maintainer still has to make the scope ruling on First Principles item 3 (the mtime stamp) and on the Design note asking for separate commits. The author has argued both on the PR.

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)

@iamwhatever

Copy link
Copy Markdown
Contributor

HOLD at 2e3d7d6, reserved for human security review.

The root-cause fix still holds: AcpProvider no longer reads chat.modelDefaults back from the shared cli.json. #19867, #14840 and #22496 are still linked for the deferred work. This head is more than a rebase, though. Since dd83aac it moves the unpinned explicit-Default handling into acp_lifecycle.py, and no PR comment describes the change.

What blocks:

  1. Merge conflict: dashboard/chat_turn/turn_admission.py conflicts with main. Please rebase.
  2. Human security review: the NON_EGRESS_REDACTION_MODULES row (security_posture.py#L494), the settings-path containment (workspace_cli_settings.py#L86-L260), and the new restart path. That path shuts down the first runtime, deletes its session files and clears the served model's entry even when Kiro Crew does not own it (acp_lifecycle.py#L111, L150-L185). Sensitive Change Review is red on this head at its human-approval step.
  3. The lane notes on this head have no reply: Design's mtime-preserving tools and lifecycle-growth notes, and First Principles items 5 and 8. Please post head-scoped dispositions, and add a short comment on what changed since dd83aac.
  4. The maintainer scope rulings from r121 are still open: First Principles item 3 (the mtime stamp) and the request for separate commits.

I read the interdiff since r121, the new lifecycle code, the lanes and CI. I did not run tests.

— crew-pr-reviewer (r123)

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on the size of the effort machinery and its spec: kept

Kept. Each piece answers a case the shared cli.json creates: the ownership keys and stamp tell Kiro Crew's writes from an operator's, and the flag carries a Default picked on a stopped chat to that chat's next start, where a fresh kiro-cli session reads the file. #22496 tracks the per-session channel that would take the shared file out of this path.
No state was added since dd83aac: the flag, its 1,000-row cap and the 409 and 503 answers are dd83aac's. The one new step, the unpinned restart, is CI GPT 6.1's upheld fix on 48bce9f.
Agreed on the spec: providers.md and session.md describe each of these pieces, the unpinned restart included.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on mtime-preserving tools and the ownership stamp, raised on 2e3d7d6: kept

The stamp is an even second at least three seconds in the past, which FAT stores exactly, so any later save by an editor or kiro-cli settings gets a newer mtime and voids the record.
rsync -t, cp -p and a backup restore copy a file's content and mtime together, so a copy reads as owned only when its content is Kiro Crew's own write. A hand edit reads as owned only when a tool such as touch -r sets its mtime back to that exact second. Kiro Crew then treats the edited level as its own and the next Default or auto start removes it from the file; it never reaches another chat.
A providers.md line naming these cases next to the kiro-cli re-verify step is on the PR's follow-up list, so the reviewed lines stay unchanged.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • First Principles item 4, the deferred explicit Default and its restart: rebutted

The flag is the in-app way to clear a level an older build wrote, which is what the leak left behind. Main stored every pick in the shared cli.json with no ownership record, so after an upgrade those entries read as hand-written, automatic Default keeps them, and no one-time cleanup can tell them from an operator's. The pre-spawn warning goes to the gateway log, so without the flag a chat whose picker shows Default keeps running them.
A Default picked on a stopped chat has no session to apply it to, so it needs a carrier to that chat's next start. The flag is that carrier; its row cap and the 409 and 503 answers are dd83aac's.
The restart is CI GPT 6.1's upheld blocking fix on 48bce9f: kiro-cli names an unpinned chat's served model only at spawn and reads cli.json once, so without it that chat runs the level it just removed, under a Default picker.
The scope ruling on the flag stays with a maintainer.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • First Principles items 6 to 9, the link refusal, the 1 MiB ceiling, UTF-8 rewrites and the Tool Search write: rebutted

Item 6: the link refusal keeps this PR's new writes inside the work directory. Where .kiro links to ~/.kiro, main's pick already overwrites the user's global level and a Default deletes it (#22821); the ownership record, its stamp and a Default carried to the next start would act on those global settings too.
Items 7 and 8: the 1 MiB ceiling bounds the whole-file read the PR's writers make under the shared settings lock at every start. With it raised, a 256 MiB file held that lock 2 s and added 641 MiB; #23396 closes with this PR. UTF-8 output keeps a rewrite under that ceiling, since escaping roughly doubles CJK text: CI GPT 6.1's upheld finding on 3d767b9. The description's What changed table declares the UTF-8 change.
Item 9: main reset an unparseable cli.json to {} and overwrote it, erasing the operator's settings; the effort and Tool Search writers now leave such a file as it is. The constructor no longer writes Tool Search: only start() does, in a worker thread after the effort projection, so a provider that is built but never started leaves the file untouched.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author

@iamwhatever

  1. Merge conflict: rebased onto main 177acfe as 4a82776. Main's refactor(dashboard): move the chat turn's finally into phases #23528 trimmed turn_admission.py's TYPE_CHECKING import list where this PR adds _apply_pending_model_pick_at_turn_start, and the resolution keeps that one name. The patch lines are identical to 2e3d7d6's.

  2. What changed since dd83aac. All of it is in the unpinned explicit-Default start.

    • CI GPT 6.1 upheld a blocking finding on 48bce9f: an unpinned start consumed an explicit Default but kept an operator's level for the served model, so the chat ran that level under a Default picker. The Default now stays pending until the backend names the served model. That model's entry is then rewritten to the chat's own level or removed, and when kiro-cli already read it the start restarts once from the same resume state (_finish_unpinned_explicit_effort_default and _restart_for_explicit_effort_default in acp_lifecycle.py).
    • The local GPT and Opus reviews then tightened that restart. The replacement gets the start's original replay flags. The step runs only for kiro-cli, the one harness that reads cli.json, and counts only the effort key kiro-cli reads. A first runtime that does not stop, or a replacement that still reads its served model's entry, fails the start and leaves the Default pending.
    • Rebases over feat: add Coder remote session environments #19833 (its remote-session refusal now sits in start_runtime()'s legacy arm), feat(chat): defer a mid-turn model pick to the turn boundary #23380 and refactor(dashboard): move the chat turn's finally into phases #23528, and Fixes #23396 for First Principles' 1 MiB ceiling note on f79b5f9.

    No durable state, endpoint or answer was added: the flag, its row cap and the 409 and 503 answers are dd83aac's.

  3. The restart path, for the security review. Each action stays inside the chat's own start.

    • It runs only after the user picks Default on that chat, on an unpinned kiro-cli start. Every automatic Default or auto start keeps an entry Kiro Crew did not write and logs it.
    • It runs inside start(), and an explicit-Default start skips the warm pool (bypass_effort), so the runtime it stops is the one that start just spawned.
    • It deletes only the session that start created, after its process has stopped, and never the session the chat resumes (first_session_id != initial_resume_session_id).
    • It rewrites or removes only the served model's entry, in the chat's admitted settings folder, under the shared settings lock and the overlay write fence. Removing an entry Kiro Crew did not write is what an explicit Default has to do there; otherwise the chat runs the operator's level under a Default picker.

    The review itself, and Sensitive Change Review's approval step, stay with a maintainer.

  4. Lane notes. On 4a82776 Design's lifecycle note reads as a note on the size of the machinery, and First Principles' items 5 and 8 are numbered 4 and 6 to 9; each has one disposition record. Design's mtime note was not raised on 4a82776, so its record names 2e3d7d6. Design note on the size of the effort machinery and its spec, Design note on mtime-preserving tools and the ownership stamp, First Principles item 4, First Principles items 6 to 9.

  5. First Principles item 3 (the mtime stamp) and the Design note asking for separate commits: the r117 and r120 replies stand, and the scope ruling stays with a maintainer. On 4a82776 First Principles lists the stamp as justified (item 2).

@iamwhatever

Copy link
Copy Markdown
Contributor

Hold at 08ad410a92e20b1e2de9c9f47047a89a86625de5, reserved for human security review.

The root-cause fix still holds, and GPT 6.1 and Opus report no findings on this head.

What blocks:

  1. Merge conflict in test/test_acp_tool_search.py against current main. Please rebase.
  2. Human security review. The PR adds a NON_EGRESS_REDACTION_MODULES row (security_posture.py#L503-L507) and settings-path containment with new pinned read and write helpers (workspace_cli_settings.py#L560-L642). Sensitive Change Review is red at its human-approval step.
  3. This head changes acp_cli_overlay.py, workspace_cli_settings.py and skill projection since 4a82776, and no comment says what changed. Please add one.
  4. Lane notes on this head have no reply: Design's three notes (kiro-cli writing cli.json itself, a start failing instead of warning, and the readers of the link refusal), and First Principles item 10 (the removed test_handles_corrupt_existing_json pin).
  5. The maintainer scope rulings from r121 are still open.

— 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.
@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on kiro-cli writing the workspace cli.json itself: measured, kept

Measured on this host's kiro-cli 2.34.0 with no model turn: /effort high and /effort medium both took effect, and neither they, a session/set_model away and back, nor the session's start and close changed a byte or an mtime under the work directory's .kiro or in the user-level cli.json. A planted mtime-only change was detected, so the probe sees one. 2.29 itself was not measured: the host now runs 2.34.0.
kiro-cli's own settings write, kiro-cli settings --workspace, leaves the file with a whole-second mtime that no longer equals the stamp, so the record reads as void and the level is kept with the warning instead of being claimed. docs/reference/kiro-cli/README.md lists that check among the re-verify steps for a kiro-cli bump, and test/test_kiro_cli_effort_ownership_pin.py fails when the bundled pin is raised past the version it was verified on.
A README line recording the 2.34.0 measurement rides the next code change.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on a start that fails instead of warning: rebutted

A start that warned and went on would run a level the chat did not pick under a picker that shows Default, which is the leak this PR fixes.
The window waits at most the settings lock's two timeouts, which bound a write in that directory. Starting anyway would let kiro-cli read the file while that write lands: the race the fence closes, pinned by TestDefaultSpawnOverlayFence::test_expired_window_fails_before_tool_search_or_runtime.
The restart path fails the start when the replacement runtime still reads its served model's entry, because that runtime would run the level under the Default picker. The failure path stops that runtime and leaves the Default pending, and the next start applies it.
A Default chat has no level of its own to push, so a post-spawn /effort push cannot stand in for removing the entry.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • Design note on the readers of the link refusal: answered in the description

The description's Backwards compatibility section lists both readers. It sits past the 8,000 bytes the lanes read. Its Breaking paragraph says that in such a work directory the effort and Tool Search writes raise and are logged and the session still starts, and that skill projection is skipped with a logged warning and the session uses its authored agents.
Its Reader line for _apply_tool_search_overlay says a refused Tool Search write is logged and the spawn continues, and names test_unmergeable_file_survives_construction_and_start and test_settings_link_outside_is_never_probed_by_any_prelock_path.

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author
  • First Principles item 10, the removed test_handles_corrupt_existing_json pin: rebutted

Main's Tool Search write reset an unparseable cli.json to {} and wrote over it, which erased the operator's settings; test_handles_corrupt_existing_json pinned that reset. First Principles itself rated the reset not justified on 935011f, and this PR fixed it as that review asked.
The write now raises and leaves the bytes as they are (test_unmergeable_existing_file_raises_and_is_left_unchanged), and the session still starts and logs the refusal (test_unmergeable_file_survives_construction_and_start).
It is declared in the description's Backwards compatibility section, past the 8,000 bytes the lanes read: "an effort or Tool Search write that finds cli.json unparseable or not a JSON object now raises and leaves the bytes as they are, where main reset the file to {} and overwrote it".

@unstablebrainiac

Copy link
Copy Markdown
Contributor Author

@iamwhatever

  1. Merge conflict: rebased onto main c24c7bc as 2dfd5d9. Main's test(acp): give weak acp tests real checks and merge twin cases #23708 merged TestInitWiring's two init tests into one parametrized test_overlay_on_init, whose kiro row expects the Tool Search write on init. This PR moves that write to start(), so the resolution keeps main's parametrized form with no call on init for either backend, and keeps the PR's test_kiro_applies_tool_search_only_during_start. Only those test lines differ from 08ad410's patch.

  2. Human security review and Sensitive Change Review's approval step: a maintainer's.

  3. What changed since 4a82776 in those files. Each change fixes a review finding, and none adds durable state, an endpoint or an answer.

    • acp_cli_overlay.py: CI Opus 5.5 blocked 4a82776 because a Default start held its work directory's effort fence while it queued for a gateway-wide cold-start slot, so a level pick in that directory could time out after 30 s and reset its chat. A Default start now waits for the directory holding nothing, queues for the slot holding nothing, takes the fence window only with the slot in hand, and hands the slot down to the spawn (_hold_workspace_effort_overlay_window_under_admission, with try_acquire_window and wait_until_writes_clear on the fence). Its pinned read and publish helpers now call the shared ones below.
    • workspace_cli_settings.py: those helpers moved here so skill projection uses the same ones: pinned_cli_settings_fd, read_pinned_cli_json and publish_locked_cli_settings. The read opens cli.json relative to the settings directory the lock holds, without following links, and refuses a link, a hardlink, a non-regular file and a sensitive path. The publish writes through that directory and raises when the folder reached by name is no longer the one the lock holds.
    • Skill projection (skill_projection_parts/inheritance_overlay.py and preparation.py): it took the settings lock but read and wrote cli.json by name, so a settings folder swapped for a link after the lock could take its write out of the workspace. It now reads and publishes through the held directory. Where descriptor-relative writes are missing, its by-name read refuses a link, a hardlink and a non-regular file, so a hardlinked cli.json is never copied into the projection. A refused or constantly changing cli.json skips the projection with a logged warning, and the session runs its authored agents.
    • Outside those files: runtime_spawn_entry.py reports a handed-down slot as granted at once, so the settings work after it counts against the startup deadline again, and acp_lifecycle.py drops a runtime/client branch no start could reach, which main's H13 test rejects.
  4. Lane notes on 08ad410, one record each: kiro-cli writes, start fails, link-refusal readers, First Principles item 10. The records name 08ad410, the head whose verdicts raised them; 2dfd5d9's verdicts do not raise them again.

  5. The r121 scope rulings: a maintainer's.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: passed Eligible automated validation passed for the current revision

Projects

None yet

3 participants