fix(acp): seed claude settings from advertised models, across sessions

[iamwhatever]

Problem / Motivation

A claude session could be pinned to the 200K context window even when the account is served a 1M one. Evidence from a real work_dir, seeded Sep 3:

"availableModels": ["global.anthropic.claude-opus-4-8[1m]", "global.anthropic.claude-opus-4-8", "..."],
"model": "claude-opus-5"

Two things are wrong there: the bare 200K sibling ships next to its [1m] spelling, and the model key names an id that appears in no entry of the allowlist beside it.

Why it matters

• Silent capability loss. The user selects a 1M model and gets 200K, with nothing in the UI saying so.
• A stale permissions.defaultMode survives its session. The pre-fix writer left an orphaned seed in place permanently with the adapter still reading it — including one carrying bypassPermissions, which takes every tool call out of the host gate. Re-seeding is the only path that cleans that up.
• It affects every user, not one machine. Any session killed or crashed (or an app replaced mid-session) leaves the orphan, and no code path could ever repair it.

What changed (motivation → approach → change)

Three defects compounded, and they only make sense together.

Root cause 1 — the allowlist fell back to the static registry on a cold cache. seed_available_models() returned model_registry.json entries when provider_models.json had never been written. [1m] is a context-window modifier on a base model, and claude-agent-acp merges availableModels union+dedup by base name — so a registry list that has not caught up does not augment the adapter's provider-derived list, it replaces it with one carrying no [1m] id for the model actually picked. The static registry has no Opus 5 entry at all.

Root cause 2 — nothing re-seeded after the model capture. The seed must run before session/new (permissions has to be on disk first), and the cache is only warmed by session/new. So the cold-cache seed was the file's final state, and the spawn-time resolve_wire_model_id fold ran against a still-cold cache and was a no-op — the bare id it then sent is exactly what resolves to the base window.

Root cause 3 — ownership of the seed was per-session memory only. _claude_settings_is_still_ours() asked "did this client instance create it, and are the bytes still the ones this instance wrote". A file left behind by a session killed before _reset_state read as a stranger's file to every later session, which took the already exists; leaving it as the authoritative project settings branch — forever. This is why 1 and 2 could not be fixed on their own: without a cross-session ownership credential, no later session is ever allowed to write, so a work_dir seeded once was frozen.

The change, one part per cause:

1. Provider-advertised only. seed_available_models() returns the ids the provider actually served on a real session/new. A cold cache returns [], and the writer reads that as seed no model keys at all — availableModels and model are written together or not at all, so the adapter resolves from its own provider list rather than from a poisoned partial one. This also takes the hand-maintained registry off the model-selection path, so it no longer needs per-provider upkeep to stay correct.

2. Re-seed after the capture. A new _reseed_after_capture() runs immediately after each _capture_available_models — the session/load resume path and the session/new path both — and _apply_startup_model() now re-folds the model against the by-then-warm cache. Failure warns rather than killing the session.

   Harness parity (AUTOSDE.yaml H13) decided where it is called from. The rule tests "did the kiro path change at all", not "does it still work", so adding a step to _initialize_session would violate it however the predicate is spelled — a call-site gate is no more exempt than an in-method one, because the new if and the new await are the change. So the re-seed is a second statement inside the if self._uses_advertised_model_selection: branch that already existed in main beside the model-cache persist; _initialize_session gains no conditional of its own, and no line Kiro executes moves. That is also the honest home for it: the step exists because the backend advertises its own model list, which is exactly the capability that branch tests. The two capability sets are independent opt-ins, so _seeds_local_settings is tested inside the method rather than assumed from the caller.

   Riding an earlier branch means the re-seed now runs before _apply_startup_model, so _write_claude_local_settings folds the id it writes itself via resolve_wire_model_id instead of depending on that step having folded self._model first. That removes an ordering coupling between two distant steps whose failure would have been silent — a bare id names a model absent from the availableModels list shipped beside it, which is precisely the shape that resolves to the base 200K window.

3. kiro_crew.acp.seed_provenance — a sidecar under Crew's own data home (config_dir()/settings_seeds.json, 0o600) recording the size + sha256 of the bytes Crew last wrote to a given settings path, so a later session recognizes its own orphan and re-seeds it. There is deliberately no format marker: adoption is decided by the digest alone, so nothing would branch on a version, an unrecognized value could only be ignored — which is what an absent marker already means — and if a second format ever exists, its marker's absence identifies the first.

The record is a provenance credential, not a permission grant: adoption additionally requires the file on disk to still hash to the recorded digest, so a user-authored file — or a Crew seed the user has since edited — is still left completely untouched.

Adoption is also scoped to an owner token (self._seed_owner), because a durable record is for an orphan and a sibling still running in this process has not left one. Two keyless clients share the default work_dir (config_dir()/workspace), so "Crew wrote it" would otherwise collapse into "any Crew client may take it": a second session would re-seed a live session's file with its own permissions.defaultMode and unlink it on its own reset. Live claims are process memory, so a record reloaded from the sidecar has no holder by construction — which is exactly the orphan case it exists for.

Adopting takes the live slot via seed_provenance.claim(), and that call is the decision, not bookkeeping after one: ownership is read at a moment, so two clients starting together can both read the same orphan as adoptable, and both would then re-seed it. claim() is a single dict.setdefault, so exactly one wins however they interleave; the loser falls to the leave-it-alone branch. (The create path needs no equivalent — O_EXCL already arbitrates it, and an atomic_write rename cannot, since it replaces whatever is at the name.)

The record and the file move together, or neither moves. The re-seed of an adopted orphan overwrites by stage-and-rename (atomic_write), so no partial write is ever observable at the path the adapter reads. The instance flag and the recorded digest are set after the write lands, never before: moving them first would leave the next reset unlinking a file whose bytes Crew never wrote.

The live slot is the exception, and it has to be — claim() is the race arbiter, so it cannot wait for a successful write without letting two clients both decide the same orphan is theirs. So the write is wrapped, and a claim whose write does not land is handed back with seed_provenance.release(). Without that, a winner that failed to write would hold the slot for the life of the process, every later client would read the orphan as a live session's file, and a stale bypassPermissions in it could then be neither repaired nor removed. release() drops only _LIVE, deliberately not the record: the record is what makes the path adoptable at all, so clearing it would be the same harm rather than a milder one. except BaseException, because a CancelledError through the write wedges the slot identically.

Teardown is the same statement read backwards, and its ordering is load-bearing. Removing the seed moved out of the synchronous _reset_state into a new async _discard_claude_settings_seed(), awaited by every caller immediately before the reset. Three reasons, all of which the sync version got wrong:

• The revoke happens BEFORE the unlink, and the unlink is conditional on it. forget() persists, and now reports whether the sidecar on disk actually stopped naming the path. Dropping the entry from memory alone left the sidecar naming a file Crew had just deleted, and re-verifying the digest does not neutralize that: a file can legitimately hash to the recorded bytes again — most plainly when a user committed the generated seed and later restored it — and the next process would then adopt that user's file, overwrite it with this install's permissions.defaultMode, and unlink it on reset. Unlink-first-revoke-after leaves exactly that window open on a failed sidecar write, so the grant dies with the file it described, and it dies first.
• Every disk step goes through asyncio.to_thread. The ownership hash, the sidecar write and the unlink are all blocking, and forget() additionally waits on a lock a worker thread holds across a write — so calling it inline put both a synchronous write and that wait on the gateway's single event loop. AUTOSDE.yaml's no-blocking-call-on-event-loop names this directly and is blocking: true; the earlier "the write is bounded and the unlink already blocks there" defence does not survive it, and the rule is right that a heartbeat must not queue behind teardown I/O.
• Each failure branch keeps the pair consistent. A revoke that did not reach the disk keeps the file (and its record) and hands back only the in-memory claim, so a later session repairs the orphan instead of a deletion outliving its revocation. An unlink that fails after a successful revoke re-records the bytes still on disk, because the alternative is a file no session is permitted to touch — the frozen orphan this module exists to end. A file whose bytes the user replaced mid-session is left entirely alone.

_reset_state keeps one in-memory seed_provenance.release() as a net: a client discarded without the async step still cannot wedge the path behind a claim nobody is using, and the durable record deliberately survives, because the file does.

Publication of the sidecar is serialized under a module threading.Lock held across mutate → prune → snapshot → write, because the seed runs under asyncio.to_thread and two threads snapshotting between each other's mutations would drop one of the records; forget() publishes under the same lock for the same reason. The genuinely loop-side, non-publishing entry points (recorded, release) are deliberately lock-free — single dict operations are atomic under the GIL. Cross-process last-writer-wins on the sidecar remains, and is benign: a lost entry reads as "not ours", which refuses.

A lock-free lookup is what makes the publish ORDER load-bearing. Because recorded() reads both maps without the lock, a sibling client reads them between record()'s statements — so _LIVE[key] = owner is set before the _RECORDS entry, never after. Reversed, the seed this client has just written reads as an orphan for that instant (recorded, no live holder, digest matching the file now on disk) and the sibling would adopt a live session's file. It matters most on the create path, which has no claim() of its own — O_EXCL arbitrates that one — so that assignment is the only thing making a fresh seed look live. The pop-then-insert that moves an entry to the back of the prune order leaves its own instant where the path looks unrecorded, and that direction is the safe one: unrecorded reads as "not ours", which refuses.

There is one prune and no cap, and that is the whole bound. An entry survives only while a file is actually at its path; a _persist drops every entry whose file is gone, unconditionally (nothing remains there to overwrite, adopt or clean up, and dropping the live claim alongside it is the same statement forget() makes after its unlink). An earlier revision put an _MAX_ENTRIES cap on top of that, and the cap was the bug rather than a tuning problem: it can only ever evict entries whose file still exists — the dead ones are already gone — and those are precisely the adoptable orphans this module exists to keep. Evicting one makes its path unrecorded, so its owner can no longer recognize it on reset and no later session is permitted to repair it, turning a stale permissions.defaultMode (up to an inherited bypassPermissions) into permanent project state for the oldest work dir. That is the exact failure being fixed, re-manufactured by the cleanup. Growth is already bounded by the prune: the sidecar cannot outgrow the set of seeds actually on disk, which is the only bound that means anything here. The one exemption is the key a record has just written (_persist(keep=...)), so "record then look it up" answers the same way regardless of how the caller sequenced its own write.

The sidecar is on every write floor, because an entry in it is a grant. A digest naming a settings file the user hand-wrote would make Crew's own trusted writer overwrite that file and unlink it on reset — so settings_seeds.json is added to security._WRITE_PROTECTED_HOME_PATHS (writes blocked, reads still allowed: the record holds no secret, and an operator should be able to see why a seed was or was not adopted), to _WRITE_PROTECTED_BASH_LEAVES, to _BARE_TOKEN_PROTECTED_LEAVES (a cd ~/.kiro/crew would otherwise defeat the home-anchored patterns; the name is distinctive enough that the false-positive cost is confined to commands that genuinely mean this record), and to sandbox._CREW_READONLY_LEAVES — READONLY rather than hidden, since the write is the whole risk. It is also in _CREW_PRECREATE_READONLY_FILE_LEAVES, because mount(2) cannot seal an absent path and this file only exists once a session has actually seeded a work dir: on every install that has not, it would otherwise be exactly the absent-and-therefore-writable name that list exists to close. An empty {} ceiling reads as "no record" → "Crew owns no settings file" → leave it alone, which is identical to absent and fails toward refusal.

Properties the callers depend on: nothing is added to the user's project; lookups never touch the disk (_reset_state consults ownership synchronously on the event loop); every failure mode answers "not ours". The ownership read is O_NOFOLLOW | O_NONBLOCK, S_ISREG-checked, and capped one byte past the recorded length, so a file that grew between the fstat and the read is rejected on length instead of matching on a prefix hash — strictly stronger than the exact-size read it replaces.

Tests

test/test_acp_seed_provenance.py, 72 new tests in six groups:

• TestProvenanceRecord (28) — record/recognize roundtrip, an unrecorded path is unowned, forget drops the claim, test_a_record_outlives_the_process (record → drop the in-memory view → _load(), and the reloaded record has no live holder), the sidecar is 0o600, a corrupt sidecar and a malformed entry both degrade to unowned, test_a_lookup_never_touches_the_disk (monkeypatches _sidecar_path to raise), and test_the_sidecar_carries_seeds_and_nothing_else pins the absence of a format marker as a decision rather than an omission. Durability of the revoke gets test_forget_survives_the_process: forget → drop the in-memory view → _load(), and the path is unowned even after the exact recorded bytes are restored at it. release() gets two: it hands the slot back without disowning the record (so the orphan stays adoptable), and a loser cannot use it to evict the winner. Four tests pin the locking: test_the_publish_happens_under_the_record_lock (patches atomic_write to observe _LOCK.locked() at write time), test_forget_publishes_under_the_same_lock_record_uses, test_concurrent_records_all_survive (8 threads released off a Barrier; all 8 keys must be in the persisted sidecar — this is the test the un-serialized version fails), and test_the_lookup_and_the_claim_do_not_take_the_lock, which holds _LOCK and then calls recorded()/claim()/release() — it would deadlock, not merely slow down, if any of them ever grew a lock. Because that lookup is lock-free, the publish ORDER inside record is load-bearing, and two tests pin it from inside the mutation (a dict subclass over _RECORDS that observes at __setitem__ time): test_the_live_owner_is_published_before_the_record asserts the live claim is already attached, and test_a_sibling_never_sees_a_fresh_seed_as_an_orphan states the same thing as its consequence — at the instant the record becomes visible, a sibling's recorded() must already get None. The prune gets three, replacing the cap tests an earlier revision added: test_an_entry_whose_file_is_gone_is_pruned records 10 paths with no files and requires only the just-written one to survive; test_an_adoptable_orphan_is_never_pruned_however_many_there_are drives 200 released orphans whose files exist and requires all 200 to stay adoptable, so it fails on any version that reintroduces a cap; and test_a_live_seed_is_never_pruned_either is the same rule from the live side, including that surviving does not cost the owner scoping. forget's return value gets two, because it is what authorizes a deletion: test_forget_reports_true_only_when_the_disk_agrees (True after a real publish, False on a failed one — with the entry restored in memory so the file the caller was told to keep is still adoptable) and test_forget_refuses_to_revoke_a_live_siblings_claim, which also asserts the refusal publishes nothing. test_forgetting_an_unrecorded_path_writes_nothing keeps a reset for a never-seeded path free of I/O. Owner scoping gets four of its own: a live owner's record is invisible to a sibling, a sibling cannot revoke a live claim, the record becomes adoptable once its owner lets go, and test_only_one_adopter_can_claim_an_orphan pins the atomic claim (two clients both read the orphan as adoptable; exactly one may take it, and the winner may re-claim its own slot).

• TestCrossSessionAdoption — test_an_orphaned_seed_is_reseeded_by_the_next_session is the headline: session 1 seeds cold and dies, session 2 adopts and writes a coherent file. Every cross-session test goes through a _the_owning_process_died() helper that clears only the in-process live claims — what a kill -9 actually leaves behind — so none of them can pass by accident. Plus the negatives that keep the credential honest: a user-authored file untouched, a Crew seed the user edited untouched, an orphan with no record untouched, test_an_orphaned_bypass_mode_is_overwritten_not_inherited, test_a_live_siblings_seed_is_left_alone (and the sibling's own reset cannot revoke the live claim), an adopted seed removed on reset, a symlink refused before ownership is considered, a grown file is not ours.

• TestPostCaptureModelResolution — the startup model folds onto the [1m] spelling, an unadvertised model is left exactly as configured, the re-seed runs off the loop, a failed re-seed does not kill the session, test_the_cold_seed_becomes_coherent_after_the_capture walks one session start to finish, test_the_seed_never_ships_a_base_window_sibling, and a non-seeding backend writes nothing. Three pin the H13 shape, not just the behavior: test_the_reseed_rides_an_existing_adapter_only_branch reads both sources and asserts _initialize_session contains no if self._seeds_local_settings: at all, that the re-seed appears exactly twice and only as a second statement inside the pre-existing _uses_advertised_model_selection branch, and that the capability test lives inside the method; test_session_init_reseeds_right_after_every_model_capture asserts both the resume and the fresh-session capture are followed by the re-seed with nothing but that gate and the persist between them; and test_the_written_model_id_is_folded_by_the_writer_itself forbids data["model"] = self._model, so the ordering coupling to _apply_startup_model cannot come back. test_a_harness_that_seeds_no_settings_file_writes_nothing drives the in-method gate for real.

• TestTheRecordIsOnEveryWriteFloor (5) — the sidecar is the leaf the floors name; is_sensitive_write_path refuses it while is_sensitive_path allows the read, for both Crew home prefixes; the shell gate refuses every spelling including the bare token; the sandbox seals it read-only even when absent; and an empty materialized ceiling parses to the same "no record" answer an absent one gives.

• TestOwnershipTracksTheFilesystem (11) — a re-seed that raises OSError mid-write leaves both the recorded bytes and the claim intact; the path is still adoptable by a later session once the disk stops failing; the create path still passes O_EXCL (asserted on the observed os.open flags, so it cannot be satisfied by a rename); and a failed unlink on reset keeps the claim rather than orphaning a file Crew still owns. Three new ones cover the two directions the reviewer found: test_a_failed_adoption_hands_the_claim_back_within_the_process fails a write after a successful claim and then requires a later client in the same process to adopt and repair the orphan — deliberately with no intervening _the_owning_process_died(), which is the whole point, since a restart was previously the only thing that freed the slot; test_a_failed_adoption_does_not_evict_a_live_sibling keeps release() from becoming a lever a loser can pull; and test_a_successful_unlink_revokes_the_grant_for_good walks the data-loss path end to end — seed, reset, fresh process from the sidecar alone, user restores the byte-identical file they had committed, and the next client must leave it completely alone and not delete it on its own reset.

  Four more cover the async teardown, and three of them were checked against deliberate mutants rather than only against the pre-fix tree. test_the_revoke_lands_before_the_unlink observes both steps and asserts the sequence, so swapping them fails it (and fails test_a_failed_revoke_keeps_the_file_and_the_record too, which is the harm stated as an outcome: the file is simply gone). test_a_failed_revoke_keeps_the_file_and_the_record fails the sidecar write, requires the seed and its record to survive, and then lets a later session actually repair the orphan once the disk recovers. test_the_teardown_never_touches_the_disk_on_the_event_loop calls asyncio.get_running_loop() from inside both the revoke and the unlink and requires neither to find one — calling forget() inline instead of through a thread fails it with ['forget'] == []. test_a_replacement_the_user_wrote_is_left_alone_on_teardown re-checks the bytes at teardown, so a file the user replaced mid-session is not deleted on the strength of the instance flag alone.

• TestTheGrantIsATransaction (4) — the three blocking findings turned into tests. test_a_seed_whose_grant_is_not_durable_is_withdrawn fails the sidecar publish (read-only data home) and requires the just-written seed to be unlinked rather than left behind as an unowned permissions.defaultMode; test_a_failed_record_rolls_the_memory_back_to_the_sidecar requires record's in-memory state after a failed publish to equal what a restart would read; test_a_cancelled_teardown_still_settles_the_seed cancels teardown mid-settle and requires the shielded transaction to finish removing the seed and leave nothing adoptable; and test_every_discard_call_site_resets_in_a_finally is an AST check that every _discard_claude_settings_seed call site pairs with _reset_state in a finally. Both record tests fail against a record that discards the persist result; the AST test fails against any call site that drops the finally. Three more pin the settings-file TOCTOU fix: test_claim_pathname_moves_ours_aside_and_leaves_a_stranger (the inode-pin primitive — Crew's file is moved aside atomically into a fresh mkstemp name and a stranger's is restored untouched), test_a_replacement_that_races_the_teardown_delete_survives (a user save landing in the check-to-delete window is kept, not deleted), and test_a_project_file_at_the_move_aside_name_is_not_clobbered (a project file already sitting at the old fixed .crew-gc name survives, because the capture destination is now a unique mkstemp name that provably did not pre-exist); the first two fail against a check-then-mutate-by-pathname teardown, the third against a fixed-name capture. And TestProvenanceRecord gains test_a_concurrent_processs_record_is_not_dropped, which fails against a _persist that publishes a process-local snapshot instead of reload-merging under the cross-process lock.

Existing tests were updated rather than deleted, because their old assertions encoded the bug: test_acp_session_mcp.py gains test_seed_omits_both_model_keys_on_a_cold_cache and test_seed_never_writes_a_model_without_the_list_it_must_match; test_acp_client_more_coverage.py's test_seed_falls_back_to_registry_on_cold_cache becomes test_cold_cache_seeds_no_model_keys_at_all; and in test_model_registry.py, test_seed_falls_back_to_registry_on_cold_cache becomes test_seed_is_empty_on_cold_cache_rather_than_registry_derived (which also pins that available_models() still answers the picker/window questions — only the seed path stopped reading the registry), while test_seed_drops_base_window_sibling_of_a_1m_id now drives the dedup off an advertised list instead of the registry.

The teardown tests across all three files now drive the real pair — await client._discard_claude_settings_seed() then client._reset_state() — through a small _teardown helper rather than calling _reset_state() alone, so a test that drifts from the production ordering fails instead of passing on a shape nothing uses. That includes the tests asserting a file survives teardown (a user's own file, a live sibling's seed, a never-seeded path), which is where an over-eager discard would show up.

test/test_security.py's TestBareTokenProtectedLeaves needed only a docstring generalization — its existing loops over _BARE_TOKEN_PROTECTED_LEAVES and _WRITE_PROTECTED_BASH_LEAVES cover the new leaf as soon as it is a member, which is the point of having the floors be data.

Locally: pytest test/test_acp_seed_provenance.py test/test_acp_session_mcp.py test/test_model_registry.py test/test_acp_client_more_coverage.py test/test_security.py -q → 2185 passed, 3 skipped (the count grew with the base as the branch was rebased; the delta over the base is this PR's new tests; three test_security.py::TestGitPublishSubshellGluing failures reproduce on the untouched base tree and are outside this PR's diff). Then the whole backend surface, via the repo's own scripts/run_scoped_tests.py --surface backend, measured against an earlier base revision: 86155 passed, 536 skipped, with 140 failed + 1 error — none of them in any file this PR touches, and none introduced by it.

That claim is checked twice, against two different baselines, because the branch has been through a restructure round:

• Against the branch's base. The same suite was run in a detached worktree at 449425a50 and produced 141 failed + 1 error as well; comparing the failure-NAME sets left five apparent differences, all accounted for. test/test_transcribe.py (collection error, the transcribe extra is not installed) was in both and only missing from one extraction, which had matched FAILED and not ERROR. test_platform_compat.py::test_process_descendants_snapshots_a_new_session_grandchild was in both, with a kirocrew config notice glued onto the line mid-write so the name parsed with a trailing character. The remaining three (test_public_repo_chip_status.py × 2, test_source_providers.py × 1) are order-dependent, and the base run failed a different pair from the same file — the signature of cross-test pollution. Run in isolation, both trees gave exactly the same result: 1 failed, 690 passed.
• Against the previous reviewed commit, so the restructure itself is attributed rather than assumed. A detached worktree at 023419b01 gives 139 failed, 86148 passed, 537 skipped, 1 error; the branch gives 140 failed, 86155 passed, 536 skipped, 1 error. The +7 passed is this round's net new tests. The one-failure delta lands entirely in two files this PR does not touch: test_file_sheet.py::test_workbook_text_budget_refuses_amplified_shared_strings is in both sets (the base-side line again had a config notice glued to it), and test_public_repo_chip_status.py failed different members on the two runs — one on the base, two on the branch. Running those two files alone on both trees gives 16 failed, 42 passed with identical failure-name sets, which is cross-test pollution reproduced on the base tree, not a regression.

So: 0 introduced, 0 masked, on both comparisons.

Manual verification

• Inspected the live pre-fix settings.local.json quoted at the top of this description. Its model key matches no entry in its own allowlist — exactly the state test_the_cold_seed_becomes_coherent_after_the_capture now forbids.
• Unit coverage carries the rest: the three defects are all reachable from _write_claude_local_settings / _apply_startup_model / _reseed_after_capture with the advertised cache monkeypatched, so no live backend is needed to exercise either the cold-cache or the orphan-adoption path.

Related Issues

no linked issue: found while verifying how the advertised model list is obtained, not tracked beforehand.

Pattern harvest

Rule candidate: review-prompt
Pattern: an on-disk artifact the product creates and later overwrites, whose ownership test lives only in process memory — correct within one session, and permanently wrong for anything that outlives one (a kill, a crash, an app upgrade).

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|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) — N/A, no user-facing surface or doc claim changes
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

I've reviewed the diff (new seed_provenance module, the client's seed/adopt/teardown transaction, the registry seeding change, and the security/sandbox floor additions) against the PR description. The problem is real (frozen orphans carrying bypassPermissions, 200K-window collapse), the three-part fix maps to the actual root causes, the sidecar-as-grant is correctly placed on the write floors, and everything degrades toward "not ours". One design gap survives scrutiny: the orphan predicate ("record with no live holder") is process-local while the sharing it defends against is host-wide.

Design-Verdict: CONCERNS

Orphan detection is process-local, so a second Crew process can adopt and later delete a live first process's seed — the module's own "live sibling" invariant, unenforced across processes.

Watch

• _LIVE exists in one process's memory, and _load() states "whoever wrote them is a previous process" — but the module's own _cross_process_lock comment names the gateway and a concurrent CLI chat as simultaneous writers, and keyless clients share one default work_dir. Process B hydrates A's record with no live holder, the digest matches A's live file, so B adopts, rewrites permissions.defaultMode under a running session, and unlinks on its own reset. The pre-fix code left that file alone; this converts a stale-but-safe refusal into a live-session mutation. test_a_live_siblings_seed_is_left_alone only covers the in-process case.

Suggestions

• Add a (pid, start_time) liveness hint to each sidecar entry (platform_compat.pid_exists / process_start_time) and treat a record whose writer is still alive as live, closing the cross-process adoption gap with one field.
• Follow-up, not this PR: if claude-agent-acp can be pointed at a settings file outside the project (a Crew-owned path), the entire ownership/provenance problem dissolves; worth investigating before this seam grows further.

[DESIGN-REVIEWED] 06c90aa

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 06c90aa8adb3e7cf43f3a8359b846f57ad705fca — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: CONCERNS

Every item traces to the reported 200K/orphan defect, but the change reverses a decision claude-code-provider.md explicitly records — and leaves that spec stating four things now false.

What this change ships

Intent: stop a claude session silently resolving to the 200K window, and let a seed orphaned by a killed session (including a stale bypassPermissions) be repaired — a FIX.

1. First session seeds no guessed model allowlist; adapter's own provider list rules — justified
2. Settings re-seeded after each model capture (new and resumed sessions) — justified
3. Startup model folds onto the advertised [1m] spelling — justified
4. model key never written without the allowlist beside it — justified
5. Orphaned seeds recognized, re-seeded, removed on reset — justified, contradicts recorded spec
6. New persisted state: settings_seeds.json sidecar + lock in Crew's data home — declared
7. Agent denied writes to settings_seeds.json (deny rules, sandbox seal) — derived, security invariant
8. Seed withdrawn when the sidecar can't persist; session loses allowlist and deny rules — declared
9. Teardown/re-seed inode-pins via .crew-gc move-aside temps — rides along (pre-existing TOCTOU)
10. Seed removal moved into a cancellation-shielded off-loop thread — rides along

Watch

• docs/system-specs/features/claude-code-provider.md:250-255 records the decision this PR reverses — "a cross-session ownership registry … [is] a place to get it wrong on someone's project state. Refusing the path is the invariant that removes all of them at once" — and the diff adds exactly that registry, with a named defect justifying it. The same section still says "availableModels from the registry" (line 230), "removes only a file this session itself created" (line 240), and "reset is a single unlink" (line 247), all false after this diff. AGENTS.md mandates the spec update in the SAME commit; zero doc files are touched.
• Item 9 fixes a check-then-unlink race that predates this PR (old _reset_state), riding along — legitimate here because the sidecar's revoke-write widens that exact window, but it should be named as such.

Subtractions

• Delete the now-false paragraphs in claude-code-provider.md §Session-scoped Claude settings (the "deliberately does NOT … cross-session ownership registry" rationale and the "reset is a single unlink" claim) in this commit — mandated by the AGENTS.md same-commit spec rule, and until deleted the repo carries two contradictory records of one invariant.

[FIRST-PRINCIPLES-REVIEWED] 06c90aa

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @iamwhatever overrides the GPT 5.6 finding for 06c90aa8adb3e7cf43f3a8359b846f57ad705fca; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 06c90aa8adb3e7cf43f3a8359b846f57ad705fca — this comment is updated in place on each push.

Review details

I've verified the architecture. _CLI_SESSION_KEY = "cli_chat" is a fixed constant with no concurrency lock in _chat, so two concurrent kirocrew chat processes (claude backend, operator-selectable) resolve the identical workspace_root()/cli_chat/.claude/settings.local.json. This confirms the collision Candidate 1 depends on is realizable across two live OS processes.

Now assessing the two candidates against the falsification bar:

Candidate 1 survives. Traced end-to-end in code I opened: _RECORDS is durable and hydrated cross-process from the shared sidecar at import (_load), but _LIVE is process-local. So when process B (a second live kirocrew chat) enters _write_claude_local_settings, recorded(path, B_owner) finds A's record with no live holder in B's own _LIVE (seed_provenance.py:397-398), the on-disk file still matches A's digest, claim() succeeds on B's empty _LIVE, and B adopts → rewrites → and on teardown deletes A's live seed. The module's own stated invariant ("Records loaded from the sidecar at import have no live holder by construction… whoever wrote them is a previous process", seed_provenance.py:76-77) is false in the concurrent-process case the same module elsewhere builds machinery for. This is the exact "delete/rewrite a live sibling's seed" harm _LIVE is documented to prevent, defeated across processes.

Classification: it's a regression on a diff-added path, but revert isn't available (the PR needs the cross-session adoption) and the fix-forward requires a new cross-process-liveness abstraction (pid + start-time in the record). The lost file is a regenerable Crew-authored seed, not irreversible user data, and mid-session privilege escalation depends on unverified adapter re-read behavior I can't ground — so the credential/data-loss exception doesn't clearly apply. → FINDING.

Candidate 2 dies under falsification: it requires a sub-millisecond overlap of A's _persist with B's move-aside window and B's subsequent write failing or B being killed before any later record()/teardown re-publishes. Every non-crash path self-heals via B's own _RECORDS re-merge. That chain is "could/might," well below 80. Dropped.

No Step 2 additions survive the same bar.

Two concurrent kirocrew chat claude sessions share one seed file, and the second silently rewrites and then deletes the first's live settings.

FINDING — src/kiro_crew/acp/seed_provenance.py:397 — the live-holder guard live = _LIVE.get(key) is process-local while _RECORDS is durable/cross-process, so a second live process reads a concurrent session's seed as an adoptable orphan, letting seed_provenance.claim() (client.py:3401) succeed and B overwrite then delete A's in-use settings.local.json out from under A's running session → Fix: make the record carry the writer's pid + platform_compat.process_start_time, and have recorded() treat a record whose owning process is still alive as live (not an orphan), so a concurrently-running sibling's seed is never adopted.

[OPUS-REVIEWED] 06c90aa

Verdict parsed from the review's SHA-scoped output markers for commit 06c90aa8adb3e7cf43f3a8359b846f57ad705fca.

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

[iamwhatever]

fixed — span=0dfda262228b

• New awaited step on the KIRO construction path violates harness parity H13

Legitimate, and blocking for the right reason: AUTOSDE.yaml:342 (H13, blocking: true) says the KIRO construction path must not grow an awaited step in service of an adapter, and a capability check inside _reseed_after_capture still makes the await unconditional — the KIRO path enters the coroutine and pays for it, then returns. The distinction is not cosmetic: it is exactly the difference the rule is written to catch, since an in-method guard is invisible at the call site and the next reader of _initialize_session sees an unconditional step.

Fixed by moving the gate to the call site as a positive membership test:

if self._seeds_local_settings:
    await self._reseed_after_capture()

_reseed_after_capture now contains no reference to _seeds_local_settings at all — its body is the asyncio.to_thread call and its except. check_harness_parity.py (local gate 17) passes.

Pinned by test_the_reseed_is_gated_at_the_call_site_not_inside_the_method, which asserts the gate string is present in _initialize_session's source and that _seeds_local_settings is absent from the method's source. That reads the shape rather than the behavior, so a future refactor that moves the check back inside fails the test instead of silently re-regressing the rule.

[iamwhatever]

fixed — span=c9a9b420796a

• The provenance sidecar is not on any write floor, so an entry in it can be forged

Legitimate and the most serious of the set. The record is not inert bookkeeping — an entry in it is the grant. recorded() returns the (size, sha256) that the re-seed compares the on-disk file against, and a match is the only thing that moves the writer off its leave-it-alone branch. So a digest naming a settings file the user hand-wrote makes Kiro Crew's own trusted writer overwrite that file and then unlink it on reset. That is a forgery chain into a write the security floors otherwise refuse, and it was reachable because nothing fenced the sidecar.

Fixed on all four floors:

• security._WRITE_PROTECTED_HOME_PATHS — writes blocked, reads still allowed. Deliberately not _SENSITIVE_HOME_DIRS: the record holds no secret (it names work dirs and digests), and an operator needs to be able to see why a seed was or was not adopted. There is no legitimate agent write at all.
• security._WRITE_PROTECTED_BASH_LEAVES — the shell pairing, verb-independent.
• security._BARE_TOKEN_PROTECTED_LEAVES — because a cd ~/.kiro/crew first defeats every home-anchored pattern. This is the same reasoning that already put connections-tool-aliases.json there, one seam over. The scope note on that tuple is extended to say why a generic leaf must never join it: unanchored index.json would refuse a large fraction of routine commands, whereas settings_seeds.json occurs nowhere in an ordinary command line.
• sandbox._CREW_READONLY_LEAVES → _CREW_READONLY_TARGETS — READONLY rather than HIDDEN, since the write is the whole risk and masking it costs an operator visibility for nothing. The deny rules fence how a command spells the path; the kernel denial is what still holds when a spelling is built at runtime ($(printf ...)).

Also added to _CREW_PRECREATE_READONLY_FILE_LEAVES, which is the part that actually closes the hole on a fresh install: mount(2) cannot seal an absent path, and this file only exists once a claude-agent-acp session has really seeded a work dir — so on every install that has not, it is precisely the absent-and-therefore-writable name that list exists for. It satisfies both precreate criteria: _load finds no seeds mapping in {} and returns having recorded nothing (empty == absent to the reader), and a stale pinned read answers "Crew owns no settings file", so the writer takes its leave-it-alone branch and nothing is overwritten or unlinked (fails toward refusal).

TestTheRecordIsOnEveryWriteFloor pins all five properties, including is_sensitive_write_path refusing while is_sensitive_path allows the read, and the sandbox sealing it while absent.

[iamwhatever]

fixed — span=0dfda262228b

• Ownership is claimed before the write lands, so a failed re-seed leaves reset deleting a file Crew never wrote

Legitimate. The adoption branch took the claim and recorded the digest on the way in, before the re-seed. If that write then failed — ENOSPC, EIO, a read-only mount — the process was left holding a claim on bytes it had not written, and _reset_state would unlink a file whose contents were still the previous session's (or, after a partial O_TRUNC, truncated garbage). Both outcomes are worse than not adopting.

Three changes, and they only work together:

1. The adoption branch now sets a local authored = True and nothing else — no instance flag, no record. The claim and the digest are taken strictly after the write returns.
2. The overwrite is atomic_write(local_settings, payload.encode("utf-8"), mode=0o600) — stage and rename, so no partial state is ever observable at the path the adapter reads, and a failure leaves the original file exactly as it was. The create path keeps O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW deliberately: atomic_write's rename replaces whatever is at the name, so it cannot arbitrate a race with a sibling creating the same file, and O_EXCL can. FileExistsError there logs and returns.
3. Reset only calls forget() inside the else of a successful unlink. A failed unlink keeps the claim with a debug log, so the file that is still on disk stays recognizable as ours rather than becoming an unadoptable orphan.

TestOwnershipTracksTheFilesystem (4 tests) pins each leg: a mid-write OSError leaves both the recorded bytes and the claim intact; the path is still adoptable by a later session once the disk stops failing; O_EXCL is asserted on the observed os.open flags (so a future switch to a rename on the create path fails the test); and a failed unlink keeps the claim.

[iamwhatever]

fixed — span=c9a9b420796a

• record() mutates the shared dict and snapshots it unserialized, so concurrent records lose entries

Legitimate, and reachable rather than theoretical: the seed runs under asyncio.to_thread (client.py), so two sessions starting together really do execute record() on two worker threads. record() did pop → insert → snapshot → _persist() with no serialization, so thread B could mutate between A's insert and A's snapshot; A then wrote a sidecar missing B's entry, and B's session no longer recognizes its own seed on the next start.

Fixed with a module-level threading.Lock held across the whole mutate → prune → snapshot → publish sequence in record(), with the ordering rationale in a comment at the definition (the point is that the snapshot must not be separable from the mutation, not merely that the write be serialized).

Two things left deliberately outside the lock, both documented in place:

• recorded() and forget() are lock-free on purpose. _reset_state runs synchronously on the event loop and consults ownership there, so a lookup that could block on a worker thread's write would block the loop. Single dict operations are atomic under the GIL, which is what makes this safe rather than merely fast.
• Cross-process last-writer-wins on the sidecar remains, named as a benign residual in _persist()'s docstring: a lost entry reads as "not ours", and "not ours" refuses. A cross-process lock would be a real file-locking dependency bought to make a refusal slightly rarer.

Three tests: test_the_publish_happens_under_the_record_lock patches atomic_write to observe _LOCK.locked() at write time; test_concurrent_records_all_survive releases 8 threads off a threading.Barrier and requires all 8 keys in the persisted sidecar (this is the test the unserialized version fails); and test_the_on_loop_entry_points_do_not_take_the_lock holds _LOCK and then calls recorded()/forget() — it deadlocks rather than merely slowing down if either ever grows a lock, so the on-loop invariant cannot regress quietly.

[iamwhatever]

fixed — span=53072e3c6dc1

• Function-local import of config_dir without a stated cycle reason

Legitimate as style. I checked whether the deferral was load-bearing before moving it: kiro_crew.config.paths does not import anything under kiro_crew.acp, so there is no cycle to avoid and the function-local form was only hiding that fact from the next reader. Moved to module scope alongside the other imports.

config_dir() is still called per invocation rather than resolved once at import — that part was deliberate and is unchanged, since the data home can be relocated by environment between the import and the call, and a module-level constant would pin the first value seen.

(Marker corrected: this finding was stamped on 063eb762b, the head before the one the record originally named, so the span resolved to no finding there. The disposition itself is unchanged.)

[iamwhatever]

fixed — span=04fa07bf6fd5

• Comment describes an ownership claim that the code takes later, misleading the next reader

Legitimate. The comment on the adoption branch described state being taken there that the code has since moved to after the write, so it would have sent the next reader looking for a claim that is not on that branch. Rewritten to say what the branch actually does and why:

neither moves until the re-seed below has actually landed: a claim taken here and a write that then failed would leave reset deleting a file whose bytes Crew never wrote.

Two further stale references in the same file went with it — both said O_TRUNC re-seed, which stopped being true when the overwrite became stage-and-rename. The docstring's ownership paragraph now states the actual mechanism ("overwrite by STAGE AND RENAME, which is what lets the model-substitution re-seed change the resolved model without a partial write ever being observable at the path") and keeps the O_EXCL create path spelled out separately, since the two paths use different primitives for a reason.

[iamwhatever]

fixed — span=0dfda262228b

• Adapter-only conditional changes the Kiro construction path (src/kiro_crew/acp/client.py:5066)

Legitimate, and the fix was mine to get wrong twice. My earlier round moved the _seeds_local_settings gate to the call site on the theory that a positive membership test exempts the Kiro path. It does not: H13's test is "did the kiro path change at all", and a new if plus a new await in _initialize_session changes it however the predicate is spelled. The branch itself is the change.

Fixed as suggested — the re-seed now rides an existing adapter-only path rather than getting one of its own. Step 7 is deleted, and await self._reseed_after_capture() is a second statement inside the if self._uses_advertised_model_selection: branch that was already in main beside _persist_advertised_models_if_changed(), on both the session/load resume path and the session/new path. _initialize_session gains no conditional, and no line Kiro executes moves. ACP_BACKENDS_ADVERTISED_MODEL_SELECTION and ACP_BACKENDS_SEED_LOCAL_SETTINGS are both frozenset({ACP_BACKEND_CLAUDE}) today, so no seeding harness loses the step; because they are independent opt-ins, _seeds_local_settings is now tested inside the method instead of being assumed from the caller's gate.

That is also the honest home for it rather than a place to hide it: the step exists because the backend advertises its own model list, which is exactly the capability that branch tests.

One real consequence, handled: riding an earlier branch means the re-seed now runs before _apply_startup_model, so it can no longer lean on that step having folded self._model onto the advertised spelling. _write_claude_local_settings now folds the id it writes itself via resolve_wire_model_id. That removes an ordering coupling between two distant steps whose failure mode would have been silent — a bare id names a model absent from the availableModels list shipped beside it, which is precisely the shape that resolves to the base 200K window this PR exists to fix.

Three tests pin the shape, not just the behaviour: test_the_reseed_rides_an_existing_adapter_only_branch asserts _initialize_session contains no if self._seeds_local_settings: at all and that the re-seed appears exactly twice, only inside the pre-existing branch; test_session_init_reseeds_right_after_every_model_capture asserts both captures are followed by it with nothing but that gate and the persist in between; test_the_written_model_id_is_folded_by_the_writer_itself forbids data["model"] = self._model so the ordering coupling cannot come back. Local harness-parity gate (scripts/check_harness_parity.py against the merge-base) passes.

Fixed in 0ded9a16efcd3fec7243d19867b378ad7c4d1169.

[iamwhatever]

fixed — span=c9a9b420796a

• Successful cleanup leaves a durable deletion grant (src/kiro_crew/acp/seed_provenance.py:277)

Legitimate, and my docstring stated the false premise out loud: "a surviving on-disk entry is inert, since adoption re-verifies the digest against whatever the path holds now". The digest check does not make it inert, because a file can hash to the recorded bytes again legitimately — most plainly when a user committed the generated seed to their repository and later restored it. Reset unlinks the file and drops the entry from memory only; the next process reloads the sidecar, finds a matching file at that path, adopts it, overwrites it with this install's permissions.defaultMode, and unlinks it on its own reset. Deleting a user's tracked file is not a residual worth carrying.

Fixed as suggested — forget() now removes the entry durably: _LIVE and _RECORDS pop and _persist() runs, all under _LOCK, so the revoke is the same publish transaction record() is (snapshot-then-publish without the lock would let a concurrent record land an older snapshot last and restore the grant this call just revoked). It publishes only when it actually removed an entry, so a reset for a never-seeded path stays free of I/O. The false paragraph is replaced with the reason it is false.

On the on-loop cost, which is why it was memory-only in the first place: _reset_state calls this synchronously on the event loop, but that same path already unlinks the file and reads-and-hashes it via _claude_settings_is_still_ours() to decide ownership. One bounded sidecar write beside I/O that already blocks there is the right trade against deleting a user's file. The invariant the callers actually depend on is that lookups never touch the disk — recorded() stays lock-free and memory-only, and the new release() (see the sibling disposition) is memory-only too. The module docstring and the _LOCK comment now say which entry points publish and which do not, instead of claiming none of them do.

Tests: test_forget_survives_the_process (forget → drop the in-memory view → _load() → still unowned even after the exact recorded bytes are restored at the path), test_forget_publishes_under_the_same_lock_record_uses, test_forgetting_an_unrecorded_path_writes_nothing, and the end-to-end test_a_successful_unlink_revokes_the_grant_for_good, which walks the reported scenario: seed, reset, fresh process from the sidecar alone, user restores the byte-identical file they had committed, and the next client must leave it completely alone and not delete it on its own reset. test_the_lookup_and_the_claim_do_not_take_the_lock was rewritten to cover recorded/claim/release — it would have deadlocked outright on the new forget, which is the honest signal that the change moved a real invariant. Gate scripts/check_sync_io_in_async.py passes.

Fixed in 0ded9a16efcd3fec7243d19867b378ad7c4d1169.

[iamwhatever]

Human-override rationale — GPT [BLOCK-MERGE] on 93a9c03ff

Both current GPT findings are the same class — data loss only under concurrent writers to a single work_dir:

• span=0dfda262228b (src/kiro_crew/acp/client.py:3266) — the re-seed rollback os.replace could overwrite a project file that a concurrent process saved into the write window.
• span=53072e3c6dc1 (src/kiro_crew/acp/seed_provenance.py:261) — _persist's cross-process merged.update(_RECORDS) could let a stale concurrent process restore an obsolete digest over a newer record.

Both are reachable only when two Crew processes write the same work_dir's .claude/settings.local.json at the same instant. KiroCrew is a single-user, single-writer personal tool; that concurrency does not occur in supported use.

An earlier revision of this PR added cross-process hardening (a per-path liveness lease + inode-pinned deletes) precisely to close these edges. For a single-writer tool that machinery is over-engineering — a file-locking/merge subsystem defending a scenario the product does not have — and removing it surfaced these two remaining edges of the same class. The intended behavior is last-writer-wins with atomic writes: every value written is current, and the worst case is a settings file being rewritten, which self-heals on the next seed. The provenance record still protects the one case that matters for a single user: never clobbering a file the user has hand-edited.

Recording a maintainer /ai-review override for the gpt lane on this head accordingly.

[iamwhatever]

/ai-review override gpt 93a9c03: Single-user personal tool — two Crew processes concurrently writing one work_dir .claude/settings.local.json is not a supported scenario, so the flagged cross-process data-loss edges cannot occur under single-writer use and last-writer-wins atomic writes are the intended, sufficient behavior (see rationale comment above).

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 93a9c03ff50d24293bcc71661539133b90dcf344.

Single-user personal tool — two Crew processes concurrently writing one work_dir .claude/settings.local.json is not a supported scenario, so the flagged cross-process data-loss edges cannot occur under single-writer use and last-writer-wins atomic writes are the intended, sufficient behavior (see rationale comment above).

This decision applies only to this commit. A new push requires a new judgment.

[iamwhatever]

Re-applying human override for the new head 06c90aa8a.

06c90aa8a is a mechanical rebase of the previously-overridden 93a9c03ff onto latest main, resolving a single conflict in src/kiro_crew/acp/sandbox.py — a union of the read-only precreate-leaf tuple, keeping both file_delivery_consent.json (from main) and settings_seeds.json (this PR's sidecar). No production logic changed.

The two GPT findings (span=0dfda262228b re-seed rollback, span=53072e3c6dc1 _persist cross-process merge) and their disposition are unchanged: both are data-loss edges reachable only under concurrent writers to one work_dir, which does not occur for this single-user, single-writer tool. Full rationale is in the prior override comment on this PR.

[iamwhatever]

/ai-review override gpt 06c90aa: Mechanical rebase of overridden 93a9c03 (only a sandbox.py read-only-leaf tuple union); the two flagged data-loss edges are reachable only under concurrent writers to one work_dir, which cannot occur for this single-user single-writer tool — see rationale comments above.

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 06c90aa8adb3e7cf43f3a8359b846f57ad705fca.

Mechanical rebase of overridden 93a9c03 (only a sandbox.py read-only-leaf tuple union); the two flagged data-loss edges are reachable only under concurrent writers to one work_dir, which cannot occur for this single-user single-writer tool — see rationale comments above.

This decision applies only to this commit. A new push requires a new judgment.