fix(artifacts): guard artifact-folder icon write-back with a per-folder epoch

[iamwhatever]

Problem / Motivation

_spawn_artifact_folder_icon_task (src/kiro_crew/dashboard/handlers/artifacts.py) guarded its async icon write-back with only an existence check (fstore.exists(folder_id)), which catches deletion and nothing else. Three stale-write-back races stayed open for ARTIFACT folders — the same three the chat-folder subsystem closes with a per-folder icon epoch:

• a manual icon set while generation was in flight was clobbered by the stale generated result;
• an icon clear mid-generation was overwritten. The icon VALUE goes absent -> absent, so a value-pin could not have caught this one either — only a counter can;
• a rename mid-generation landed an icon derived from the old name. Because an artifact-folder rename regenerates, a rename followed by a manual pick also lost to the regenerated result.

Why it matters

The user's own explicit choice silently loses to a background LLM result, seconds after they made it. There is no error and no retry affordance — the icon they picked just turns into a different one, and the only recovery is to notice and pick again. The handler even carries a comment asserting that "an explicit icon wins", which the async path could violate.

What changed (motivation → approach → change)

Root cause: nothing carried a per-folder version, so the write-back had no way to tell "the folder still wants this icon" from "the user has since acted". exists() answers a different question.

Approach: port the chat-folder guard (_CHAT_FOLDER_ICON_EPOCHS / _bump_icon_epoch in src/kiro_crew/dashboard/chat_folders.py, PR #7353) to the artifact folder store, preserving its ordering contract:

Step	Under the store lock?
Epoch bump on icon set / clear / rename	Yes — inside set_icon / rename, same critical section as the field write
Epoch capture for arming	Yes — returned from the mutation; never read back
Epoch check + icon write-back	Yes — both inside set_icon_if_epoch, one critical section
Epoch pop on delete	Only after _save() confirmed the removal

Concretely:

• ArtifactFolderStore gains self._icon_epochs, _bump_icon_epoch_locked() and set_icon_if_epoch().
• set_icon bumps — one call site covers both a manual set and a clear (icon == "").
• rename bumps and returns (folder, epoch) — the epoch its own bump produced.
• delete pops the affected ids (whole subtree on cascade) after _save(), so a failed commit keeps the guard armed.
• The PATCH handler arms generation with the epoch rename returned. The create path pins 0.

Arming has to be atomic with the mutation — this is the subtle half, and the first revision of this PR got it wrong (caught by the GPT lane; see the disposition comments). rename() followed by a separate epoch read is two lock acquisitions: a manual icon set landing between them bumps the epoch again, the later read captures that epoch, and the generated icon then satisfies set_icon_if_epoch and overwrites the user's pick — reintroducing exactly the race the epoch exists to prevent. Returning the value from inside the bump's own critical section closes the window by construction: there is no read to lose. The create path pins 0 for the same reason — a fresh id has no registry entry (and delete pops, so even a reused id reads 0), and a read there could only pick up a competing bump.

No epoch getter ships, deliberately. Those two paths are the only ways a caller obtains an epoch, so a public read-back accessor would have had zero production callers — and the read it enables is the bug above. An earlier revision did add one; the First Principles lane flagged it as a rider and it is gone (see its disposition). Tests read the registry through a local helper instead. For the same reason rename() is the single spelling of that mutation rather than a dict-returning wrapper sitting beside a tuple-returning twin, which would drift.

Two deliberate deviations from the chat-folder original, both because the artifact side is a store object rather than DashboardState:

1. The epoch is per store INSTANCE, not module-level. ArtifactFolderStore is constructed per JSON path (and tests construct their own over tmp_path); a module-level dict keyed only by folder id would let two stores alias each other's ids. Locked to self._lock, which is the lock that orders the bump against the check.
2. set_icon_if_epoch is a store method rather than a callback. Chat folders mutate through state.mutate_folders(callback), so the epoch check can live inside the caller's callback. ArtifactFolderStore exposes discrete lock-taking methods, so the check-and-write has to be one method to stay in one critical section.

The exists() pre-check is removed rather than kept alongside the epoch: it was itself a TOCTOU gap (the folder could vanish, or its icon change, between the check and the write), and re-finding the folder under the lock subsumes it. That also drops a lock acquisition from the event loop inside the background task.

Note on PR #7353

#7353 is still open, so _CHAT_FOLDER_ICON_EPOCHS is not on main yet. This PR is nonetheless self-contained and lands on main as-is: the artifact-side bug is real on main today (see the red-before results below), and #7353 touches neither src/kiro_crew/artifacts.py nor src/kiro_crew/dashboard/handlers/artifacts.py, so there is no file collision and no ordering dependency. The pattern was read off #7353's branch to match it.

Worth flagging for whoever reviews #7353: the late-capture defect found here applies to its shape too — it captures _CHAT_FOLDER_ICON_EPOCHS.get(fid, 0) after mutate_folders(_apply) returns, which is the same two-step read this PR had to replace.

Tests

test/test_artifact_folder_icons.py, mirroring test/test_chat_folder_icons.py. 27 tests across both layers.

Store layer (TestStoreIconEpoch):

• fresh folder and unknown folder read epoch 0;
• set_icon bumps; an icon clear bumps (the case a value-pin cannot see); rename bumps;
• set_icon_if_epoch writes on a matching epoch, and does not bump (a generated result landing is not a user mutation);
• set_icon_if_epoch drops a stale write, and drops a write for a deleted folder;
• a confirmed delete pops the entry; a cascade delete pops every subtree entry;
• a failed delete commit keeps the guard armed, and the stale result still loses;
• epochs are per store instance; an unrelated folder's mutations do not invalidate a pending generation;
• rename returns its own bump, and that returned value is exactly what a read-back would have lost — the atomicity contract.

Handler layer (TestIconRacesThroughHandlers) — the races end-to-end through the real handlers:

• create generates and writes the icon; a failed generation leaves the folder unchanged; a folder deleted mid-generation is not resurrected;
• manual icon set mid-generation is not clobbered;
• icon clear mid-generation is not overwritten;
• rename mid-generation drops the stale old-name icon;
• rename-then-manual-pick keeps the manual icon (the case named in the issue);
• a manual pick landing inside the arming window is not clobbered (the GPT finding);
• a manual pick before a created folder's icon lands wins;
• an explicit icon in the same PATCH as a rename wins and arms no generation;
• a delete through the handler releases the epoch.

Red-before proven per mechanism, separately. With the two source files reverted and the tests kept, the four original races fail as genuine clobbers, not as missing API:

FAILED test_a_manual_icon_set_mid_generation_is_not_clobbered  - AssertionError: assert '🚀' == '🧪'
FAILED test_an_icon_clear_mid_generation_is_not_overwritten    - AssertionError: assert not '🚀'
FAILED test_a_rename_mid_generation_drops_the_stale_icon       - AssertionError: assert '🚀' != '🚀'
FAILED test_a_rename_then_manual_pick_keeps_the_manual_icon    - AssertionError: assert '🧬' == '🧪'

With only the late epoch capture restored (everything else fixed), the arming-window test fails on its own:

FAILED test_a_manual_pick_in_the_arming_window_is_not_clobbered - AssertionError: assert '🧬' == '🧪'

Test-hygiene notes, both fixing real CI reds this PR hit: the drain helper filters in-flight tasks to the current running loop, because _ARTIFACT_FOLDER_ICON_TASKS is module-global and other test files populate it via the create handler — gathering the whole set awaits futures from earlier tests' closed loops (ValueError: The future belongs to a different loop, 7 shard failures). And a _held_generation context manager owns the release Event and drains in a finally, so an aborting assertion cannot leave a task parked on it.

Local gates: the baselined black gate (scope clean, no baseline churn), isort, flake8, mypy, check_sync_io_in_async, check_testpaths_coverage, check_loop_bound_locks, check_brand_name, check_harness_parity all pass. 486 tests green across the six artifact suites.

Manual verification

N/A — unit coverage sufficient. Every race here is timing-dependent and is driven deterministically instead: an asyncio.Event holds the generator open while the competing mutation lands through the real HTTP handler, and the arming-window case hooks _run_off_loop so the competing pick lands the instant the rename commits. That is more reliable than reproducing them by hand.

Screenshots / video

Why no screenshot: backend-only change (store + its handler). The rendered glyph is unchanged; it just stops being the wrong one.

Pattern harvest

Rule candidate: semgrep

Pattern: a versioned-guard counter (epoch / generation) read in a separate lock acquisition from the mutation that bumped it, then used as the expected value for a later compare-and-set. The read can observe a competing writer's bump, so the stale actor's write satisfies the comparison and lands — silently defeating the guard it was supposed to arm. The counter must be returned from inside the mutation's own critical section; a subsequent read-back is never equivalent, which is why this change ships no accessor for it.

This is the defect the GPT lane caught in this PR's first revision, and it is worth a rule precisely because the code reads as correct: the bump is under the lock, the check is under the lock, and only the capture in between is not.

Closes #7991

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix at the right layer: a CAS epoch owned by the store's own lock, with the arming value returned from the mutation's critical section.

Suggestions

• The cited precedent (_CHAT_FOLDER_ICON_EPOCHS, PR feat(folders): restore auto-generated emoji icons for chat folders #7353) is unmerged, so the docstring pointer in artifacts.py ("Ported from the chat-folder guard _CHAT_FOLDER_ICON_EPOCHS") dangles at HEAD; reword it to describe the invariant without naming a symbol that isn't in-tree, and when feat(folders): restore auto-generated emoji icons for chat folders #7353 lands, consider one shared guard helper so the two copies can't drift.

[DESIGN-REVIEWED] e5af344

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

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

All verification is done. The mechanism claimed as "already existing" (_CHAT_FOLDER_ICON_EPOCHS) is genuinely absent from main (grep: only this PR's own comments reference it — #7353 is unmerged), so there is nothing to reuse; on main the artifact path is the only consumer of generate_emoji_for_name, so no sibling write-back is left unfixed. rename's tuple return and set_icon_if_epoch each have exactly one production consumer, and both are the fix's mechanism, not generalization. The removed exists() pre-check is a subtraction. Here is the review:

First-Principles-Verdict: PASS

A user's explicit icon choice no longer silently loses to a stale background LLM result; every added piece is the guard or its atomicity carrier.

What this change ships

Intent: stop a background-generated folder icon from overwriting what the user did meanwhile (pick, clear, rename) — a FIX.

1. A manually picked artifact-folder icon no longer flips to a generated one — justified (red-before proven)
2. Clearing an icon mid-generation now sticks — justified (absent→absent defeats any value check; counter is derived)
3. A rename mid-generation drops the old-name icon; rename-then-pick keeps the pick — justified
4. Delete-mid-generation guard moved from a racy pre-check into one locked section — justified (TOCTOU closed)
5. rename() now returns (folder, epoch) — mechanism; 1 consumer (handlers/artifacts.py:3002), atomicity-derived
6. New set_icon_if_epoch + in-memory _icon_epochs — mechanism; 1 consumer (handlers/artifacts.py:2892)
7. exists() pre-check deleted — subtraction, declared
8. Epoch entries popped only after a confirmed delete commit — declared, harm named

No duplicate mechanism exists: grep _CHAT_FOLDER_ICON_EPOCHS across src/ hits only this PR's own comments (#7353 is unmerged, and its folders live on DashboardState, not a store). No unfixed siblings: generate_emoji_for_name has exactly one write-back path on main — this one. The fix sits at cause level (no version accompanied the write-back), the epoch is the smallest state that distinguishes clear and rename from no-op, and the deliberately withheld epoch getter keeps the surface at its honest minimum.

[FIRST-PRINCIPLES-REVIEWED] e5af344

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e5af344

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

[iamwhatever]

• span=f7a17f6ef6ce — Late epoch capture can clobber a newer manual icon — fixed in d3a2de2f3.

Legitimate, and the more interesting of the two: the guard was correct at both ends and wrong in the middle. Confirmed reachable exactly as described.

Late epoch capture can clobber a newer manual icon

The PATCH handler bumped the epoch inside rename() (under the store lock) and then read it back with icon_epoch() — a second lock acquisition. A manual set_icon landing between the two bumps again, so the read returned that epoch, the generation was armed with it, and set_icon_if_epoch then found the epoch unchanged and wrote the generated icon over the user's pick. The epoch was arming itself against the wrong value.

Fix: the mutation now returns the epoch its own bump produced, from inside the same critical section — rename_and_icon_epoch(). There is no read-back left to lose. rename() delegates to it and keeps its existing dict-returning contract, so no other caller changes. The create path pins 0 by construction for the same reason: a fresh id has no registry entry (and delete pops, so a reused id also reads 0), and a read there could only ever pick up a competing bump.

Red-before proven on this mechanism alone. With everything else fixed and only the late capture restored, the new test fails as a real clobber — the generated emoji overwriting the manual one:

FAILED test_a_manual_pick_in_the_arming_window_is_not_clobbered - AssertionError: assert '🧬' == '🧪'

The test drives the window deterministically by hooking _run_off_loop so the competing pick lands the instant _apply_updates commits.

Recorded as a rule candidate in the PR's ## Pattern harvest section, because the shape is general and reads as correct on inspection: a versioned-guard counter read in a separate lock acquisition from the mutation that bumped it, then used as the expected value for a later compare-and-set.

[iamwhatever]

• span=8e3b8fe2a006 — comment claims the folder is left present, but _folders is filtered before _save() — fixed in d3a2de2f3.

Legitimate. The comment was wrong about the state a failed _save() leaves behind.

"leaves the folder present" contradicts _folders being filtered before _save(), so a failed save leaves it absent in memory -> Fix: correct or remove the claim.

self._folders is reassigned before _save(), so on a raise the folder is gone from memory and survives only on disk. Corrected the comment to describe that split honestly, and to state why keeping the epoch is still the right side of it: the on-disk record comes back on any later reload, and resetting its epoch to 0 would let a stale in-flight generation clobber a manual icon on the record that returns.

Code behaviour is unchanged — the pop placement was already correct, only its justification was inaccurate. test_a_failed_delete_commit_keeps_the_epoch_guard pins it, and its docstring is corrected to match.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] e5af344

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

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

[iamwhatever]

• span=f7868914cde7 — Held icon tasks leak when assertions abort — fixed in 9d66bd4ac.

Accepted on hygiene grounds, and applied at the chokepoint rather than per call site. One correction to the stated consequence, below.

assert patched.status == 200 precedes release.set() and task draining.
handler regression -> assertion aborts -> pending task remains strongly referenced across event loops
Fix: release and drain each held task in a finally block.

The structural point holds: five tests parked a generation task on release.wait() and only released it after their assertions, so an aborting assertion skipped both the release and the drain. Fixed with a _held_generation async context manager that owns the release Event, the generator patch, and a finally that always releases and drains. Put it at the chokepoint deliberately — patching the five sites individually would have left the next one to be found in another round.

Where the finding overstates the consequence: I probed it rather than assuming. A temporary test that deliberately fails inside a held block, followed by a test asserting the module-global set holds no pending task, passes even with the pre-fix shape — the parked task is cancelled at event-loop teardown and its done-callback discards it from the set. So the "remains strongly referenced across event loops" step did not reproduce, and this was not on the path to the cross-loop ValueError fixed earlier in this PR (that one came from tasks whose discard callback never ran because the loop was already closed).

Keeping the fix anyway: the finally makes the cleanup deterministic instead of dependent on loop-teardown semantics, it satisfies the no-test-side-effects anchor, and it removes five copies of the same release-and-drain tail. Recording it as fixed rather than rebutted because the code did change and the hygiene point was fair — but the severity was Low, not blocking, and I would not have widened the diff for it on the stated consequence alone.

28 tests in the file still pass, and the sibling suites (test_artifact_folder_handlers.py, test_handlers_artifacts_coverage.py, test_artifact_folders.py) are green alongside it — 218 together.

[iamwhatever]

• icon_epoch() is a rider with zero production callers — fixed in e5af34417.

Legitimate, and the count is exactly right. Verified before cutting: grep -rn "icon_epoch(" src/ found the definition, the handler comment, and nothing else — create pins the literal 0 and rename uses its own returned value, both deliberately avoiding the read.

Delete icon_epoch() (src/kiro_crew/artifacts.py:3562). Its docstring says "Callers capture this when SCHEDULING a background icon generation," but no caller does.

Deleted. The tests read the registry through a local _epoch(store, fid) helper, whose docstring records why the getter is absent rather than leaving the next contributor to re-add it: a public read-back accessor would not merely be unused, it would expose the precise operation this PR exists to prevent. The epoch is safe to obtain in exactly two ways — returned from the mutation that bumped it, or pinned to 0 on create — and a getter invites the third.

Also took the subtraction, in the same commit:

Fold rename() into rename_and_icon_epoch(): post-change, \.rename\( on this store counts 0 production callers besides the delegation itself. Two spellings of one mutation will diverge.

Collapsed to one method named rename(), returning (folder, epoch). Kept the plain name rather than the compound one — rename_and_icon_epoch described the implementation, and with the dict-returning twin gone there is nothing to disambiguate against. Two call sites updated (test/test_artifact_folders.py).

Worth noting what this shed: the previous revision had repointed test_handlers_artifacts_coverage.py's error-mapping test at rename_and_icon_epoch, because that was what the handler called. With rename restored as the only spelling, that repoint is unnecessary and the file drops out of the diff entirely — 5 files down to 4. A subtraction that makes the diff smaller than the version that prompted it is a good sign the premise gate was reading the right thing.

486 tests green across the six artifact suites; mypy, flake8, isort and the baselined black gate all clean.

[iamwhatever]

• span=4499f6d61a01 — "read by the caller AFTER" contradicts the atomic epoch return and create-time pinning — fixed in e5af34417.

Legitimate. The docstring was left describing the mechanism the previous round replaced.

"read by the caller AFTER" contradicts the atomic epoch return and create-time pinning -> Fix: describe both capture mechanisms accurately.

"Read by the caller AFTER its own mutation committed" was true of the revision that had the late-capture bug, and became false the moment the epoch started coming back from inside rename's own critical section. Neither path reads it: the rename path takes the value rename() returns, and the create path pins 0. Rewritten to name both, and to say why neither is a read — a read-back would be a second lock acquisition that could capture a competing mutation's epoch.

Also corrected the neighbouring _apply_updates comment, which still pointed at the now-deleted rename_and_icon_epoch by name.

Advisory, and the fix is comment-only — no behaviour change, no test change.

[chenmingwei23]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix(artifacts) - per-folder icon epoch closes a stale-write-back TOCTOU race on artifact-folder icon generation (ported guard, issue #7991), clear root cause, no auth/trust-boundary/input-parsing surface.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #5979 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5979: REBASE. The merged fix supersedes exactly one of the primary's seven change sites and edits the identical lines, which is the source of the 'merge conflict' label; the rebase drops that hunk and the icon-task test (or retargets it at set_icon_if_epoch) and keeps the remaining nine offloads. Files: src/kiro_crew/dashboard/handlers/artifacts.py.

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