fix(artifacts): optimistic-concurrency token for content PATCH (409 on stale write)

[peterhieuvu]

Problem / Motivation

PATCH /api/artifacts/{slug} has no stale-write protection: api_artifact_update passes body["content"] straight into ArtifactStore.update(), which writes unconditionally. Last write wins. Two dashboard windows open on the same artifact silently clobber each other today; a user's stale buffer silently overwrites an agent's iteration; and for file-backed artifacts, an external write to source_path is invisible to an editor holding a stale buffer. The frontend artifactEditGuard is page-local and does nothing across windows or surfaces.

Neither existing field can detect a stale base: version is not bumped by silent saves (snapshot: false, the dashboard default), so two different live states can share a version number; updated_at is bumped by metadata-only renames/retags, so it false-positives on non-content changes.

Why it matters

This is the same stale-snapshot-clobber class already recognized on the agent-config PUT surface (#7470, #7089) — the artifacts PATCH instance of it. The blast radius is user data: the overwritten content is someone's live edit. It gets worse as write frequency rises — any client implementing debounced autosave of live state multiplies the race windows. The store itself already treats this as a real problem for remote pushes (PublicationMetadata.last_pushed_sha256, sent as expectedCurrentSha256, is described in source as "the optimistic-concurrency guard"); local writes had no equivalent.

What changed (motivation → approach → change)

Symptom → root cause: unconditional content writes with no way for a caller to say "only if nothing changed since I read." Approach: opt-in optimistic concurrency mirroring the store's own remote-push design, with the compare inside the store lock (a handler-level compare would be TOCTOU-racy against concurrent store writers).

• ArtifactStore.update() gains optional expected_sha256. When provided with content, live content is read exactly the way get() reads (source file for file-backed artifacts, current.html otherwise), hashed, and compared under self._lock before any mutation. Mismatch raises the new ArtifactConflictError, which carries current_sha256 + version.
• File-backed artifacts get a second layer: the store lock only excludes store-mediated writers, so an external process can rewrite source_path between the compare and the mirror. For guarded writes the source mirror runs FIRST, as a descriptor-pinned compare-and-swap through the existing verified_replace_file_nolink primitive (base_hash=expected_sha256, max_bytes=MAX_CONTENT_BYTES); conflict/too_large abort before current.html or metadata are touched. _try_write_source_path now returns the verdict string instead of a bool; all call sites updated. Unguarded writes keep today's last-write-wins + demotion-to-source_copy_only semantics exactly.
• The handler maps ArtifactConflictError → 409 with {error, current_sha256, version} so the client can refetch and re-base; a non-string token is a 400 (caller bug), not a phantom conflict.
• to_dict includes content_sha256 whenever content is included — computed on raw content before the HTTP serializer's redaction pass, because the guard hashes the raw bytes on disk. Only computed on detail/update responses; the list path serializes without content and pays nothing.
• ArtifactDetailPage adopts the token: handleSave sends expected_sha256 from its last fetch, and a 409 renders as a "changed since you loaded it" notice that preserves the buffer (editedContent untouched, editing stays true) and refetches so the next save carries the fresh token — a deliberate overwrite instead of a silent one. New i18n key in all 13 catalogs (en-XA regenerated).

Token omitted = today's behavior; zero existing callers change (MCP, revert, pull/publish, blank-settlement all send no token).

Review-round hardening (rounds 2–3):

• The client token is captured at edit start and held for the edit session (a background refetch mid-edit can no longer authenticate a stale buffer); it moves only via a successful save's response or a 409's body.
• For guarded file-backed saves, ONE raw read feeds both sides: decoded text for the client token, the raw-byte hash for the mirror's compare-and-swap — a source with non-UTF-8 bytes saves cleanly instead of 409ing forever.
• Guarded saves are all-or-nothing: every non-ok mirror verdict aborts with 409 before any store write. The unguarded path keeps its demote-to-source_copy_only fallback unchanged.
• The 409 banner has its own title ("Save refused — content changed") and a "View the newer content" link that opens the live version in a popout without leaving the edit buffer.
• Declared behavior change: event_type validation moved to the top of store.update() (before any side effect), so an invalid event_type on a non-snapshot save is now a 400 instead of being silently accepted — previously it could also raise mid-write and leave a save partially applied.

Round-4 hardening (guarded mirror ordering). The guarded save now writes the store's own copy first and mirrors to the source second (CAS). Consequences, both deliberate:

• No compensation path ever rewrites the source file. A failed save can no longer touch source bytes at all — in particular a non-UTF-8 source can never be rewritten through a lossy decode. "conflict"/"too_large" mirror verdicts restore the store copy and answer 409 with nothing applied.
• A "refused" mirror (read-only file, foreign owner, path outside its authorizing root — no competing writer) no longer 409s: it falls through to the pre-existing demote-to-source_copy_only path, keeping the edit and visibly detaching the artifact from the unwritable source. The previous behavior was a permanent 409 loop, since the 409 body re-based the client onto the same unwritable file.

Tests

Store (test/test_artifacts.py, TestConflictToken): matching token writes; stale token raises with recovery fields and writes nothing; omitted token keeps last-write-wins; token ignored on metadata-only updates; a silent save trips a stale token despite an unchanged version number (the reason the token is a hash, not the version); an external source-file edit trips the guard; a source write racing in between the compare and the mirror answers conflict instead of clobbering (CAS layer); unguarded mirrors still last-write-wins; to_dict carries/omits the token correctly. Existing _try_write_source_path tests updated to the verdict-string contract.

Handler (test/test_artifacts_handlers.py): stale token → 409 with current_sha256/version and content unchanged; matching token saves and returns the fresh token; malformed token → 400 not 409; detail response includes content_sha256.

Component (website/src/test/ArtifactDetailPage.saveConflict.test.tsx, new — with a controlled Pierre editor stub so a buffer re-seed regression fails visibly): Save sends the token; 409 shows the conflict notice, keeps the buffer, and refetches; non-409 errors keep the generic error path.

Manual verification

Live two-writer race in an isolated pod: opened the artifact, dirtied the editor, issued a silent save from "another window" via the API, clicked Save → 409 banner with the buffer intact; clicked Save again → informed overwrite applied with the fresh token. Screenshots below are from that run.

Screenshots / video

Stale save refused — banner, buffer preserved, Save still armed:

"View the newer content" opens the live version in a popout without leaving the edit buffer

Related Issues

Fixes #7751

Pattern harvest

Rule candidate: review-prompt
Pattern: "content-write endpoint accepts no base token — concurrent writers silently clobber (compare against #7470/#7089 class); a compare outside the writer's own lock, or a mirror to an externally-writable file without CAS, reopens the same hole"

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) — the PATCH /api/artifacts/{slug} row in docs/system-specs/modules/artifacts.md updated with the token, the 409 body, the 400 on a malformed token, and content_sha256 on content-carrying responses
• No secrets, credentials, or internal references in the diff

[peterhieuvu]

CI status note: the only red is Coverage Combine → "Upload combined coverage", which failed with a GitHub artifact-service error, not a test failure:

##[error]Failed to FinalizeArtifact: Received non-retryable error: Failed request: (403) Forbidden: Error from intermediary with HTTP status code 403 "Forbidden"

All test lanes on the run are green (backend, frontend, lint, build); Coverage Gate then fails closed because coverage-combine != success, which takes PR Readiness with it and holds back the AI review lanes (they trigger on CI success).

Could a maintainer re-run the failed jobs on run 33600280562? I'm holding off pushing anything — the diff is unchanged and a push would only re-arm the fork approval gate for the whole suite, which is more maintainer work than a re-run click.

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of acf8cbddfe41d44790153616cb6f6cab7800d2d9 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've reviewed the patch against the base tree (the verified_replace_file_nolink CAS primitive it builds on, the store's read/mirror paths, and the repo's screenshot convention). Final review:

Design-Verdict: CONCERNS

Sound, well-layered guard on a real lost-update class; one mirror verdict ("too_large" on an already-oversize source) still forms the permanent-409 loop the PR itself outlawed.

Watch

An oversize file-backed source makes a guarded save permanently unsavable with a false message. Cause: _try_read_source_bytes truncates at MAX_CONTENT_BYTES, so for a source strictly larger than the cap the guard's source_base_hash is a truncated-bytes hash that can never match the full file the CAS reads → verified_replace_file_nolink answers too_large every time → the diff's mirror_verdict in ("conflict", "too_large") branch 409s with "source file changed while saving", and the rebased token (hash of the same truncated view) reproduces the loop on every retry. No competing writer exists — the same species the round-4 hardening moved refused out of the loop for ("aborting would strand the user in a permanent 409 loop"), yet too_large-at-read-time stays in it, with a misleading banner. Net still safer than the old silent tail truncation, but the failure story for this case is wrong.
Clears when: a test covers a guarded save on a source file > MAX_CONTENT_BYTES, and that case either follows the refused demotion path or surfaces a distinct "source too large to write back" error instead of the conflict 409.

Suggestions

• Classify the mirror verdicts by their own criterion — "does this verdict imply a competing writer?" — rather than by name: conflict is the only one that does; too_large at read time belongs with refused.

[DESIGN-REVIEWED] acf8cbd

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/artifacts.py:1792 -- guarded replacement still clobbers a racing external save
verdict = hooks.verified_replace_file_nolink(
External rename after the helper’s final stat -> guarded update overwrites the newer inode -> external edits are lost.
Anchor: residual/crash-data-loss-corruption
Fix: use a truly conditional atomic replacement, or refuse guarded source mirroring where unavailable.

BLOCKING -- src/kiro_crew/artifacts.py:2076 -- failed rollback leaves rejected content persisted
with contextlib.suppress(Exception):
CAS conflict plus rollback write failure -> stale edit remains in current.html -> fallback later serves rejected content.
Anchor: residual/crash-data-loss-corruption
Fix: stage the store-copy replacement so a conflict never requires best-effort rollback.

FINDING -- src/kiro_crew/dashboard/handlers/artifacts.py:1828 -- "refused before any mutation" contradicts the CAS-conflict path, which writes and rolls back current.html -> Fix: distinguish pre-write mismatches from post-write conflicts.

[BLOCK-MERGE] acf8cbd
[GPT-REVIEWED] acf8cbd

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I've traced both fenced findings against the base code and the pre-fetched diff. Here is my adjudication.

F1 — artifacts.py:1792 (verified_replace_file_nolink call). The residual is the read-to-rename data race inside _pinned_replace: an external atomic-save (new inode) landing between the last-moment identity/mtime re-check (hooks.py:2824, hooks.py:2831, hooks.py:2838) and the rename (hooks.py:2851). Harm rung: HIGH (single external edit lost), but the window is a handful of instructions deliberately placed last (hooks.py:2807-2816), the limitation is POSIX-inherent — the stdlib exposes no conditional rename, documented as a narrowing-not-guarantee at hooks.py:2818-2822 — and this exact residual already exists in the base's unguarded mirror path, which the guarded path strictly improves on. Remedy (renameat2/RENAME_EXCHANGE via ctypes, Linux-only and still unclosable on macOS/Windows, or disabling guarded mirroring entirely) is disproportionate. Record complete → FLAG.

F2 — artifacts.py:2076 (contextlib.suppress). Requires a triple fault: CAS returns conflict/too_large (hooks.py:2837/2849), the restore write self._write_text(prev, prior_snapshot) then fails despite the same file's write succeeding moments earlier (diff ~2078), AND later the source pointer becomes unreadable so get() falls back to current.html (artifacts.py:1494-1504). Recovery: self-corrects on the next successful save, and when served the artifact is flagged source_missing=True + live_dirty=True (artifacts.py:1503, 1519-1524) — visible, not silent; the stale content is the user's own rejected edit, not cross-user data or a secret. Harm rung: LOW-MEDIUM, recoverable, visible, self-correcting. Record complete, rarity/visibility argument holds → FLAG.

[ADJUDICATION] acf8cbddfe41d44790153616cb6f6cab7800d2d9 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] acf8cbddfe41d44790153616cb6f6cab7800d2d9
[ADJUDICATION-FENCED] acf8cbddfe41d44790153616cb6f6cab7800d2d9 fenced=2 flagged=2
FLAG F1 src/kiro_crew/artifacts.py:1792 -- The lost-update window is a handful of instructions between the last-moment re-check (hooks.py:2824-2838) and the rename (hooks.py:2851), a POSIX-inherent race the stdlib cannot close (renameat2 unexposed), already present and accepted in the base unguarded path that this CAS strictly improves on.
FLAG F2 src/kiro_crew/artifacts.py:2076 -- Serving rejected content needs a triple fault (CAS conflict + restore-write failure right after the same write succeeded + a later dead source pointer), the stale content is the user's own edit, get() flags it source_missing+live_dirty visibly (artifacts.py:1503,1519-1524), and it self-heals on the next successful save.
[GPT-ADJUDICATED-FENCED] acf8cbddfe41d44790153616cb6f6cab7800d2d9

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

• F1 src/kiro_crew/artifacts.py:1792 — The lost-update window is a handful of instructions between the last-moment re-check (hooks.py:2824-2838) and the rename (hooks.py:2851), a POSIX-inherent race the stdlib cannot close (renameat2 unexposed), already present and accepted in the base unguarded path that this CAS strictly improves on.
• F2 src/kiro_crew/artifacts.py:2076 — Serving rejected content needs a triple fault (CAS conflict + restore-write failure right after the same write succeeded + a later dead source pointer), the stale content is the user's own edit, get() flags it source_missing+live_dirty visibly (artifacts.py:1503,1519-1524), and it self-heals on the next successful save.

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of acf8cbddfe41d44790153616cb6f6cab7800d2d9 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/CONCERNS are advisory.

All verification passes cleanly: the CAS primitive is reused (not duplicated) from hooks.py:2891, the same stale-clobber class is already guarded on the sibling prompts PUT surface (prompts.py:1941), the screenshots directory is an established repo deliverable, the defect has a linked issue plus base-failing tests, and every capability in the diff is declared in the description. Here is the review:

First-Principles-Verdict: PASS

Verify #7751 actually reports this clobber — the fix's provenance rests on that link plus the base-failing TestConflictToken tests, both checkable.

What this change ships

Intent: stop two concurrent writers (window/agent/external file edit) from silently overwriting each other's artifact content — a FIX.

Inventory (10 items)

1. A save can carry a base token; a stale one is refused with 409 plus recovery fields — justified
2. Content-carrying artifact responses now include content_sha256 — justified
3. Dashboard Save sends the token; a 409 shows "Save refused — content changed", keeps the draft, refetches — justified
4. "View the newer content" opens the live version in a popout without leaving the edit buffer — justified
5. Invalid event_type on any save now rejected before any write (400) instead of mid-write or silently accepted — justified
6. Guarded file-backed saves mirror to the source as a compare-and-swap; a racing external write 409s instead of being clobbered — justified
7. A guarded save to an unwritable source demotes to a detached copy instead of a permanent 409 loop — justified
8. A malformed token is a 400 caller bug, not a phantom conflict — justified
9. API spec row and 13 locale catalogs updated — justified
10. _try_write_source_path returns a verdict string instead of a bool (all call sites updated) — justified

[FIRST-PRINCIPLES-REVIEWED] acf8cbd

[github-actions]

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of acf8cbddfe41d44790153616cb6f6cab7800d2d9 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

Solid conflict-recovery copy and flow, but fork-lane screenshots are unviewable and two recovery paths leave the user under-informed at the point of action.

Watch

• After a 409, the unchanged Save control (Cmd+S) silently becomes "overwrite the newer content" — the banner ("saving again will overwrite the newer content") is the only signal, so a muscle-memory second Cmd+S destroys the other writer's unversioned live state. Low frequency × data-loss impact × once-per-conflict. Smallest fix: while saveConflict is armed, restate the action on the save affordance ("Overwrite newer content").
• The guarded "refused" mirror path returns a successful save while the on-disk file is untouched and the artifact permanently stops mirroring (source_copy_only) — source_copy_only appears nowhere under website/src, so the "visibly detaching" claim in the PR body is API-only; the dashboard user reads it as a normal save and their linked file silently diverges forever after. Rare × silent-divergence impact × persistent. Smallest fix: a one-line notice when the update response flips source_copy_only.

Evidence gaps

• "Save refused — content changed" banner (title, body, preserved-buffer layout) — conflict-409-banner.png is added by the fork and not materialized here; no blind reader has seen it. Push the branch to this repo to run the blind read.
• "View the newer content" button and the popout it opens — view-newer-content-popout.png likewise unviewable; same fix.

[UX-REVIEWED] acf8cbd

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I've traced the candidate through the actual code paths.

The candidate's mechanism is real at the code level: for a source file over MAX_CONTENT_BYTES, _pinned_replace returns "too_large" before the hash compare (seen > max_bytes), the store raises ArtifactConflictError, and the 409 body's current_sha256 is the sha of the same truncated read the client already holds — so a rebase reproduces the identical 409.

But it fails the validation bar on two independent grounds:

• (a) does not occur in practice at 80+. The dashboard promotion path (_authoritative_promote_content) reads allow_truncate=False and refuses any file over MAX_CONTENT_BYTES at creation (artifacts.py:1172, 1943), so a file-backed editable artifact cannot be minted from an oversize source. The only route to the state is a source that grows past 25 MiB after linking — an uncommon precondition for a text/markdown editor artifact, which is why the discovery pass itself rated it "low." I cannot re-derive this as an input that occurs in ordinary use.

• (c) is not a clearly wrong outcome. When the live source genuinely exceeds the cap, the CAS only ever verified a truncated prefix; refusing the write is the fail-closed choice. The pre-PR alternative (unguarded write) would have truncated the 25 MiB+ file down to 25 MiB — actual silent data loss. The new behavior preserves the file and keeps the user's buffer; the harm is a save dead-end, not loss, corruption, or a crash. That is not in the BLOCKING class, and the refusal itself is defensible.

No new grounded findings emerged from the surrounding code (the */function glue at ArtifactDetailPage.tsx is style; the event_type validation move is a hardening; the all-or-nothing mirror ordering and token rebase logic are sound and well-tested).

No findings.

[OPUS-REVIEWED] acf8cbd

[peterhieuvu]

• fixed span=a1f47bacb15c — Source-first CAS can leave a failed save partially applied — fixed in 638025f

Writable source + failed current.html write -> source changes, PATCH returns 500, metadata and fallback remain stale.

The window is real and is now closed on both edges, in 638025f67:

1. The deterministic trigger is gone: event_type validation is hoisted to the top of update(), before the mirror and every other side effect. Previously it sat inside the snapshot branch, so a rejected request could raise after the source write (this also fixes the pre-existing orphaned-versions/vN.html variant the old in-branch comment described).
2. The nondeterministic trigger is compensated: on the guarded path, a current.html write failure after a successful source CAS now restores the source to its pre-save content and re-raises. The rollback is itself a CAS against the content we just wrote, so if an even-newer external write has landed in the meantime the rollback loses its race and correctly leaves the newer content in place. Locked in by test_store_write_failure_rolls_back_guarded_source_mirror and test_invalid_event_type_validates_before_any_write.

On the prescribed fix ("revert the source-first write"): reverting would reopen the exact hole the CAS exists to close — with current.html written first, a conflict detected at the source write would leave the losing writer's content in the store's fallback (served whenever the source later becomes unreadable), and the pre-CAS ordering allowed an external write landing after the guard's compare to be silently overwritten. Source-first + compensation keeps both invariants: a losing writer changes nothing, and an external winner is never clobbered — including by the rollback itself.

[bolichen97]

Description / code mismatch

One item does not line up: the checklist asserts no module doc covers this endpoint, but docs/system-specs/modules/artifacts.md documents PATCH /api/artifacts/{slug} and is not updated for the new request field, the new response field, or the new 409.

1. Documented PATCH contract changes (new request field, new response field, new 409) with no spec update, and the Description states the opposite

The Description says —

• Documentation updated (if applicable) — behavior documented in docstrings; no module doc covers this endpoint today

The code does — docs/system-specs/modules/artifacts.md:142 carries a PATCH /api/artifacts/{slug} row for this endpoint, so a module doc does cover it. The diff changes that endpoint's contract in three documented-surface ways and leaves the row untouched: the handler accepts a new optional expected_sha256 request field, rejects a non-string token with 400, and maps ArtifactConflictError to a 409 carrying {error, current_sha256, version} (src/kiro_crew/dashboard/handlers/artifacts.py); and to_dict adds content_sha256 to every content-carrying response (src/kiro_crew/artifacts.py). Docstrings are not a substitute here, because the spec row is the documented contract for this endpoint.

Risk — AGENTS.md:218 requires updating the spec in the SAME commit when an API, schema, or documented behavior changes. The artifacts spec is what the next contributor reads before touching a content-write path or writing a second client; leaving it silent means they will not know a token exists, that a 409 is now reachable, or that content_sha256 is part of the response contract. The checklist line makes this worse rather than neutral: it asserts there is no module doc to update, which removes the one prompt a reviewer would use to catch the omission. Docs Lint only validates index integrity, so no gate catches it either.

Required change — Extend the PATCH /api/artifacts/{slug} row at docs/system-specs/modules/artifacts.md:142 in this commit with: the optional expected_sha256 token, the 409 outcome and its {error, current_sha256, version} body, the 400 on a non-string token, and content_sha256 on content-carrying responses. Correct the checklist line "no module doc covers this endpoint today" to name the spec update. Store-side prose for ArtifactStore.update(expected_sha256=), ArtifactConflictError, and the file-backed compare-and-swap layer is welcome but optional, since that doc does not currently enumerate update()'s parameters or the other artifact exception classes.

[peterhieuvu]

You're right on both counts — the spec row exists and my checklist line asserted the opposite, which is exactly the kind of claim that hides the omission. Fixed in d77288726:

• docs/system-specs/modules/artifacts.md — the PATCH /api/artifacts/{slug} row now documents the optional expected_sha256 token, the 409 outcome with its {error, current_sha256, version} body, the 400 on a non-string token, and content_sha256 on content-carrying responses (noting GET /{slug} carries it too).
• PR description checklist line corrected to name the spec update instead of denying the doc exists.

I kept the store-side prose (update(expected_sha256=), ArtifactConflictError, the CAS layer) out of the doc per your "optional" note, since that page doesn't currently enumerate update()'s parameters or the exception classes — happy to add a paragraph if you'd prefer it documented there as well.

[peterhieuvu]

• fixed span=a1f47bacb15c — Guarded writes accept a refused CAS race — fixed in 39226df

External atomic save after verification -> CAS returns refused -> stale content is saved and the artifact silently detaches from the newer source.

Fixed as prescribed, and — since this is the third round on this span — generalized to the invariant that makes all three rounds' failure modes unreachable rather than another point-patch: a guarded save either fully applies or changes nothing. On the guarded path every non-ok mirror verdict now aborts with ArtifactConflictError (409) before current.html or metadata are touched — conflict/too_large keep their changed-while-saving message, refused gets a could-not-safely-replace message. The demote-to-source_copy_only fallback is now reachable ONLY from the unguarded path, where it keeps today's semantics exactly; the guarded path can no longer store a stale buffer or silently detach from a newer source under any verdict. Locked in by test_guarded_refused_mirror_conflicts_instead_of_demoting (refused → 409, source intact, store content intact, live pointer NOT demoted).

The trade this makes explicit: a guarded save of a genuinely unwritable source (read-only file) now refuses loudly instead of silently demoting and diverging — the honest behavior for an endpoint whose contract is "only if nothing changed."

[peterhieuvu]

Round-3 findings addressed in 39226dfac (one batch):

Opus — encoding mismatch between the token and the CAS base (blocking): fixed. The guard now performs ONE raw read of the source and derives both representations from it: the decoded text (what clients see — the token compares against this) and the raw-byte SHA-256 (what the mirror's compare-and-swap verifies at the descriptor). A source with non-UTF-8-round-tripping bytes saves cleanly instead of 409ing forever; locked in by test_non_utf8_source_saves_cleanly_under_guard (a latin-1 é on disk, token from the decoded read, guarded save succeeds and lands). The rollback CAS keeps the decoded-text hash deliberately — those bytes are our own UTF-8 write, so the hashes coincide there (comment in source explains).

UX — half-informed overwrite (concerns): fixed. The 409 now renders under its own title ("Save refused — content changed" — protection, not a fault) and carries a "View the newer content" link that opens the artifact's popout window, which renders the live version — inspectable without cancelling the edit buffer the banner promised to keep. Screenshots in the PR body show both.

First Principles — undeclared event_type validation move (concerns): declared. The PR body now names it as a deliberate behavior change: an invalid event_type on a non-snapshot save is a 400 up front instead of silently accepted — the hoist exists because late validation could fire after the source mirror and leave a "failed" save partially applied to the user's file.

[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

• This PR is OVERLAPPING with PR #7199. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7818: CONTINUE_DEVELOPMENT. Different user goals on shared files. Both should land; whichever merges second needs a small textual rebase in the exception block, the handler import block, and the ArtifactDetailPage banner stack. 7199 is also based on a newer merge base (63a043a) than 7818 (259db39). Files: src/kiro_crew/artifacts.py, website/src/pages/ArtifactDetailPage.tsx, 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.

[peterhieuvu]

Round-4 findings addressed in 4e99f39f6 (one batch, rebased onto today's main; the earlier merge conflict against #7177's to_dict change was also resolved in passing — both changes kept).

GPT span=a1f47bacb15c (rollback corrupts non-UTF-8 sources) + Opus span=3add4de86a64 (guarded refused = permanent 409 loop) — fixed together, with a reordering rather than a smarter rollback. The two findings attack the same design from opposite ends: the round-3 invariant needed a source rollback (which round 4 correctly showed re-encodes a non-UTF-8 source through the lossy decode), while its all-or-nothing treatment of "refused" strands a readable-but-unwritable source in a retry loop (the 409 body re-bases the client onto the same file, so the next save refuses identically). The guarded save now writes the store's own copy FIRST and mirrors to the source SECOND (still the descriptor-pinned CAS against the guard's raw-byte hash):

• The source is written exactly once — the CAS install of the new content — and no compensation path ever writes the source, so a failed save can never alter source bytes (GPT's corruption scenario is structurally unreachable, not patched around). This is not a return to the round-1 shape: round 1's half-applied save was store-commit-then-plain-write; here a lost mirror restores the store copy (an internal UTF-8 file, no byte-fidelity concern) and answers 409 with nothing applied.
• "conflict" / "too_large" — the verdicts that imply a competing writer or an unverifiable swap — keep the all-or-nothing contract.
• "refused" carries no competing writer (read-only file, foreign owner, path outside its authorizing root), so it now falls through to the pre-existing demote-to-source_copy_only path: the edit is kept and the artifact visibly stops claiming the source tracks it (source_copy_only flips in the PATCH response, plus a WARNING log) — addressing round 3's "loudly, not silently" concern without the 409 loop. Declared in the PR body as a deliberate behavior change from round 3.

Tests: test_guarded_refused_mirror_demotes_and_keeps_the_edit (inverted from round 3's assertion, comment explains why), test_store_write_failure_leaves_the_source_untouched, and new test_guarded_conflict_restores_store_copy_and_never_rewrites_source_bytes (non-UTF-8 source comes out of a failed save byte-identical).

GPT span=2f715e40b5e4 (raw <button> bypasses the Btn primitive) — fixed. ConflictViewLiveAction now renders the shared Btn (default variant), dropping the bespoke link styling. Screenshots re-captured live and re-pinned.

GPT span=5bb284628476 + Opus span=6504b7c8bc2e (conflict affordance misbehaves inside a popout window) — fixed, one change for both. The render site is now gated on !popout, matching its sibling ArtifactPopoutControl: inside a popout the useArtifactPopouts subscription is documented main-dashboard-only and isPoppedOut(slug) reads false for the window's own slug, so the action would have spawned a duplicate window. The banner itself (with the kept-draft promise) still renders in popouts; only the popout-spawning affordance is main-window-only.

Verification: 437 backend artifact tests, mypy/flake8/isort clean, the repo's own baselined black gate passes, tsc, 166 frontend tests, eslint at the zero-warning cap, i18n catalog parity across all locales, production build. Evidence screenshots re-captured from a live instance at this head.

[bolichen97]

Rebased onto main 9af9543b by a maintainer as part of the 2026-09-08 open-PR audit.

Conflicts: none. Clean rebase, single commit reapplied as acf8cbdd. The diff is byte-identical in shape to the old head (24 files, +979 / -85), so nothing about the PR's behaviour changed.

Gates run locally on the rebased head (changed files only): black (all four touched Python files are in .github/black-baseline.txt, so unchanged there), isort, flake8 clean; pytest test/test_artifacts.py test/test_artifacts_handlers.py -> 437 passed, 2 failures that also fail on plain origin/main on this host (test_copy_outside_allowed_roots_is_refused, test_request_cannot_nominate_its_own_root) and are unrelated to this PR; tsc --noEmit clean; the new ArtifactDetailPage.saveConflict.test.tsx -> 4 passed.

Please review the rebased head. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong.