Skip to content

fix(chat): persist undrained pending context across a close - #6813

Open
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/persist-pending-context-across-close
Open

fix(chat): persist undrained pending context across a close#6813
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/persist-pending-context-across-close

Conversation

@rnoack1

@rnoack1 rnoack1 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

slot._pending_context is in-memory only. Nothing serializes it, and the close path pops the
slot from state._slots, so undrained background context is discarded silently.

A producer is told the write succeeded — POST /api/chat/slots/{slot}/context and .../note
both return 200 — and then the content disappears with no trace on any surface: no transcript
row, no log line, nothing a user could go and find. /note at least leaves its visible half
behind, so its content survives in the transcript. /context is context-only by design, so its
content is lost outright.

Reachable by ordinary use: an app or a scheduled job queues context, the user closes the tab
before sending their next message, and the queued content is gone. Reopening from History does
not recover it, because resume constructs a fresh slot with an empty queue.

Why it matters

It is silent loss of data the system acknowledged. The 200 is a promise the close quietly
breaks, and nothing downstream can detect it — the queue reads empty afterwards,
indistinguishable from never having been written.

The asymmetry inside the existing /note endpoint is the clearest evidence this is unintended
rather than a deliberate trade-off: that endpoint takes real care to make its visible half
durable (its docstring calls the visible line "permanent"), while the context half it writes in
the same call has no durable representation at all.

The same gap loses context across a gateway restart, not just a user-initiated close.

Reviewing without network access: the three frames below are committed to this branch, so they can be opened from the checkout rather than fetched. They live at temp-screenshots/artifact-context-notice/after-01-generic.png, after-02-queue-full.png and after-03-first-injection.png.

Declared behaviour changes (riders)

Twelve behaviour changes ride along with the durability fix. Each is detailed under Declared
riders and accepted risks
near the end of this body; they are ALSO listed here because that
section sits past the 8 KB point at which some review tooling truncates a PR body, so a reader
limited to the first 8 KB would see no declaration at all.

  1. /context can now answer 429 context_not_queued. The per-slot queue used to evict its
    oldest entry, so the endpoint always answered 200; it now REFUSES instead, because eviction
    destroyed content the API had already acknowledged. A caller that treated 200 as
    unconditional must handle 429. /note does NOT answer this 429: it still writes its
    visible line and reports contextSkipped: true, so only the silent half is affected. (/note
    has its own unrelated, pre-existing 429, deferred_notes_full.)

  2. An entry posted with no maxAge now expires. DEFAULT_CONTEXT_TTL_SECS gives the queue
    a seven-day backstop where such an entry previously had no expiry of any kind. This is what
    stops a refusing queue staying refused forever.

  3. The artifact companion's context is now durable with a 1 h TTL, where it was previously
    ephemeral (memory-only, withheld from the metadata line). This is the change under test
    rather than a side effect, but it IS a changed default for that caller and is declared as
    one.

  4. A persisted linked_session_key is trust-checked at hydration. It was previously adopted
    verbatim at both hydration sites. An unprovable value now leaves the slot UNBOUND, so this
    rider's blast radius is CHANNEL ROUTING, disjoint from context durability. Known limit,
    measured rather than argued: a key carrying a literal _ exactly where the transcript stem
    has one is refused, because it is byte-identical to the impostor shape the gate exists to
    refuse. See test_a_legitimate_literal_underscore_key_is_refused_and_that_is_measured.

  5. DEPRECATED: the app-kit "re-post your context on reconnect" recipe. A caller
    following it now DOUBLE-INJECTS, because queued context survives the close it used to be
    lost to, and nothing server-side detects the repost -- there is no idempotency key on
    /context. MIGRATION: drop the reconnect repost and rely on the queue surviving, or key
    the repost on the caller's own marker. The in-repo caller is adapted; an external one is
    not reachable from inside the gateway. docs/app-kit/api-reference.md states this.

  6. /context accepts an optional contextKey, and honours it. A POST naming a key an
    UNEXPIRED entry from the same source already carries is a no-op returning the ordinary
    { ok, pending }. It exists because the queue now survives a close, so a caller reposting
    after a reload would otherwise deliver the same content twice with no recovery until the TTL.
    Omit it and nothing is deduplicated. Declared in docs/app-kit/api-reference.md; the only
    in-repo consumer is the artifact companion, which keys on the artifact version.

  7. Papyrus's co-author context now carries maxAge 3600. It previously had no per-entry
    expiry, so a co-author session left idle over an hour now loses its document context where
    before it never expired. The agent re-reads the document on demand, so the recovery is a tool
    call rather than a lost turn -- but it IS a changed default for that surface.

  8. ephemeral keeps its true default, so an omitted flag stays MEMORY-ONLY. The pre-existing contract is unchanged: no caller that omits the flag begins writing to disk. Durability is opt-IN — a caller passes ephemeral: false to have an entry persisted to the session metadata line and re-seated after a close or a gateway restart, and only the literal boolean false opts in. Both in-repo callers pass it explicitly, so this rider changes no caller's behaviour. Declared in docs/app-kit/api-reference.md beside the options list.

  9. _disk_meta_key fixes a pre-existing "every save aborts after a rebind" defect. Neither
    cosmetic nor optional: without it the durable copy is not writable after a rebind, so the
    fix this PR exists for would not hold. Declared because it changes behaviour on a path this
    PR did not otherwise need to touch.

  10. A new user_messages count on slot state and the slot projection. The artifact page's
    stale-context notice keyed its retraction on the total message count, so the agent's OWN reply
    retracted the notice before the user had said anything. The count of USER turns is a new field
    rather than a client-side derivation because the client sees counts, not roles. Declared because
    it adds a field to a payload other surfaces read. It exists ONLY to serve rider 12: without
    the notice there is nothing whose retraction needs a user-turn count, so the two ride or fall
    together.

  11. Over-budget pending context now spills to a sidecar file beside the transcript, and a
    session delete can REFUSE.
    Entries that do not fit the metadata line are written to
    context-overflow/<stem>.jsonl rather than dropped, which is a new on-disk artifact with its
    own lifecycle: it is folded back on the reads that opt in, pruned after each commit, and
    removed with the transcript. delete_session now returns False when that file can be neither
    removed nor quarantined, because deleting the transcript while it survives leaves the deleted
    session's context hydratable by the next session at the same key. Declared because it adds a
    file to the sessions directory and a refusal path to an operation that previously always
    proceeded. The sidecar is NOT on the common path: a save with nothing over the budget and no
    existing spill writes no file at all, so the per-slot ceiling (~484 KB against a 5 MB line
    budget, needing 11+ same-transcript holders to spill) does not buy an unconditional write on
    every save that merely holds queued context.

  12. Two new user-visible failure notices, across all 13 catalogs. A /context post that
    fails now SAYS so on the surface that made it: the artifact page renders
    chat_context_notice_title (first share failed) or chat_context_stale_notice_title (a
    refresh failed, so the agent holds an older version) with a dismiss control, and Papyrus
    renders the co-author equivalent. Each carries its own body, and the stale one promises the
    LATEST version rather than a first share. Required rather than optional: the repo's
    errors-use-error-notice rule mandates that a user-initiated failure reach an
    ErrorNotice rather than only a console line, and silent loss on this exact path is what
    this PR exists to end -- a durable queue that fails quietly would replace one silent loss
    with another. Declared because it adds prose to every catalog and a surface a first-time
    reader meets only on failure. The notice retracts on the next USER message, which is what
    rider 10 is for.

What changed

Persist the live entries on the slot save; re-seat them on every restore path, all through one
restore_pending_context so they cannot drift.

All four hydration sites

A slot-owned key is hydrated in four places — the sites that hydrate channel_folder_filed:

Site Function
chat_persistence.py _rehydrate_slot_from_history (gateway restart, cron delivery)
chat_persistence.py _apply_recent_session (recent / foldered / pinned restore)
chat_handlers.py api_chat_slot_resume (History reopen)
channel_slots.py surface_channel_session (channel reconcile)

All four seat the key, because absence of a slot-owned key means "cleared". A site that
hydrates an empty queue makes the next forced save delete the stored copy — losing context
rather than merely failing to restore it. That reaches recent/foldered/pinned sessions and
reconciler-surfaced channel slots, which the Slack thread backfill shares this queue with.

This is also why the save writes the key on every save rather than only when closed: a
non-close save that omitted it would clear a copy an earlier close had written. Writing it
unconditionally also covers a crash between the enqueue and the next message.

Concurrent drain vs. flush

The save runs in an executor thread while the drain runs on the event loop, so a flush can
export the queue, the loop can drain it, and the write would then persist entries already
handed to the model — with a crash before the next save re-injecting them.

The slot carries a generation counter that the drain bumps, captured before the export and
re-checked immediately before the metadata line is serialized. A mismatch deletes the key,
which is also the correct end state: a drained queue should clear the stored copy. Appends
deliberately do not bump it — persisting a subset is safe and self-correcting, persisting a
consumed entry is not.

Retirement is carried by the flush, not by a per-turn forced save

The drain bumps the generation but deliberately does NOT mark the slot dirty -- arming the periodic flush there would race the very window the generation check exists to close -- and the flush persists the
emptied queue. An earlier revision instead forced an fsync'd durable save on every
context-consuming turn and awaited it before the drained text was prepended, with
shield/cancel/requeue handling around it.

That ordering was removed. Committing the emptied queue before delivery opens a window where a
crash between the commit and the prompt reaching the model leaves zero copies of content a
200 already acknowledged, which is the failure this change exists to prevent — the rebind
retirement's own comment calls a zero-copy outcome "strictly worse than a duplicate". Leaving
the retirement to the flush inverts the residual to at most one duplicate injection, which a
restart recovers from and a deletion does not. It also removes an fsync per context-consuming
turn, ~120 lines of cancellation repair, and a model-visible failure prepend.

TTL arithmetic hardened at its own site

context_entry_expired computes injectedAt + maxAge, which a metadata line carrying
"maxAge": "60" turns into int + str. Untreated, that 500s the resume, and on the restart
path raises into a broad handler that pops the slot — so the whole tab silently fails to
restore.

Fixed inside context_entry_expired rather than only at the restore, so every caller is
protected: the drain, the per-source cap count, and the deferred-note promotion. A
present-but-invalid value reports EXPIRED rather than "never expires", so unparseable data is
pruned instead of made immortal — which also retires a NaN maxAge that previously compared as
non-expiring forever. The restore additionally skips such entries so garbage never occupies a
seat.

Choosing that site over a restore-only guard is deliberate: it is where the unsafe arithmetic
actually lives, and the boundary validators (_validate_max_age) only ever guarded the live
enqueue.

Cross-session authorization

A note stamps both halves, and the save is a late resolution point for the queued one. The
persist now applies the same _note_authorized_elsewhere filter this function already applies
to the message window, with the matching count-gated note_save_drop denial. Without it, a slot
rebound after the write copies one session's note content onto another's metadata line
silently — while the visible half of the same note is dropped and audited.

The restore drops foreign-authorized entries for the same reason. Unstamped entries are
untouched: /context and the Slack backfill share this queue and record no session.

Size bound

The live ceilings permit _MAX_PENDING_CONTEXT (50) entries of _MAX_CONTEXT_CONTENT (40 000)
chars — two million characters, reachable through the documented API with no tampering — and it
would all land on the session metadata line.

_maybe_rotate can only drop message lines. Once the metadata line alone exceeds
_SESSION_MAX_BYTES, its budget loop floors at one kept message, and since _maybe_rotate runs
from append, every subsequent message archives the rest of the transcript. The persisted
payload is now bounded well below that ceiling, and the bound is enforced at the DOOR rather than
at save time: append_pending_context REFUSES an entry that would not fit, and /context answers
429 context_not_queued. Nothing is truncated after acknowledgement -- an earlier revision dropped
oldest-first at export, which meant a caller could be told 200 and then lose its content with no
surface reporting it. A refusal is visible and retryable; silent truncation is neither.

Source label

source is validated on restore against the same rule the HTTP boundary applies: the drain
interpolates it into [Background context from "<source>"], so a crafted label could forge a
frame boundary and make injected content read as a separate trusted block. An unusable label is
removed and the content kept, letting the drain's own default name it.

Expiry

Wall-clock across the close. The restore routes through append_pending_context, so maxAge
and both per-slot ceilings are applied by the same code that governs a live enqueue. FIFO
eviction is GONE — this diff removes it, and a full queue now answers 429
context_not_queued instead of silently dropping the oldest already-acknowledged entry. An
entry with no maxAge never expires, which is what makes the wedge below possible.

Deliberately not changed

A context prefix that has been drained still does not appear in the transcript. That
exclusion is intentional — see the raw-message capture for the Slack mirror in _run_chat, and
the silent-consumption contract added in #4780 after an agent recited injected context verbatim
into a reply. Persisting the drained prefix would reintroduce exactly that.

There is a residual fidelity cost: a reopened session shows a user message whose reply was
shaped by text absent from the record. Addressing it collides with the silent-context promise,
so it is left out of scope rather than traded away here.

Not covered

slot._deferred_notes — a held note's context half awaiting end-of-turn promotion. It is a
turn-in-flight structure on a path the close already cancels, and folding it in would widen this
change past the data loss it fixes.

Also noted and not chased: _live_slot_resume_response matches by slot name, else by
effective_session_key, and its own comment documents a channel-key mismatch producing two tabs
for one conversation. In that state both would seat the same entries. I could not construct the
mismatch, so I can assert only that the dedup this relies on is documented as having a hole.

Compatibility: callers that re-post on reconnect

The queue is now durable, so a caller following the old "re-post it if it must survive a
restart" recipe supplies a second copy alongside the restored one and the model sees the content
twice. docs/app-kit/api-reference.md states the required change (drop the re-post for
/context and for the context half of /note).

Measured blast radius in this repo — three call sites use api.chatSlotContext:

Call site Regresses?
website/src/pages/ArtifactDetailPage.tsx (two sites) Yes. Guarded only by an in-memory injectedVersionRef, which a page reload clears, so a reload re-posts alongside the restored copy.
website/src/apps/papyrus/PapyrusPage.tsx No. Posts once against a freshly created slot, so no restored copy can exist.

Worth noting for callers: durability on /context and /note is opt-IN — ephemeral keeps its true default, so an omitted flag stays memory-only and a caller passes ephemeral: false to have an entry persisted. Only the literal boolean false opts in. This is declared in docs/app-kit/api-reference.md. Worth noting for callers: ephemeral: true now keeps an entry memory-only. It queues, drains
and expires like any other, but export_pending_context withholds it, so it never reaches the
metadata line and does not survive a close or a restart. It bounds the QUEUE's durability, not the
conversation's: once an ephemeral entry is actually injected, its content is in the transcript.

Tests

test/test_pending_context_survives_close.py — 107 tests over a real ConversationLog, so
they exercise the actual metadata line rather than a mock of it.

  • All four hydration sites, with resume driven over HTTP — every other round trip reaches
    the restart path, so the resume call site needs its own coverage.
  • The drain-vs-flush race, hooking the export so the drain lands in the window the generation
    check exists to catch. Verified to fail when that check is disabled, on exactly the
    persisted-key assertion, with a guard asserting the drain really fired inside the save.
  • An append asserted not to invalidate a pending export, so the fix is not vacuous on a busy
    slot.
  • Malformed timing fields ("60", [1], True, NaN, Inf, bad injectedAt) asserted to leave
    the session resumable — a 200 from the endpoint and a restored tab — not merely to avoid
    raising.
  • A hostile source asserted not to forge a second frame in the rendered drain output.
  • Foreign-authorized entries asserted absent from both the persist and the restore, while
    unstamped entries survive.
  • The byte bound asserted against the payload and against the session cap, including that an
    over-budget entry is REFUSED at the door (and that a held note's context half is reserved, so
    later /context cannot squeeze out content already acknowledged).
  • source, maxAge and injectedAt asserted to round-trip, not just content.
  • Clearing after a drain, and an ordinary session's metadata line gaining no key at all (with a
    positive control on the same read so the absence cannot pass vacuously).

Regression: 5690 passed / 7 skipped across the channel, persistence, slot, restore, resume,
hydrate, metadata and rotation suites.

Gates: baselined black, isort, flake8 (zero new findings, verified by per-file
base-vs-HEAD counts), and mypy src/kiro_crew/ clean across 1167 files.

One existing stub in test_slack_mirror_context_leak.py gained the new counter field. That file
already documents the convention: the drain's requirements are satisfied at the stub rather than
guarded in the drain, because making the drain tolerate an incomplete slot would silently
disable the protection for every such caller.

Pattern harvest

Rule candidate: flag a request handler that answers 200 for caller-supplied content whose only storage is an in-process structure no persistence path writes.

That is precisely the shape of this defect. Two endpoints accepted background context, returned 200, and appended it to an in-memory queue; closing the tab dropped the slot and took the queue with it, so content the API had already acknowledged was gone with no error anywhere. Nothing connected "we told the caller 200" to "this survives an ordinary lifecycle event", which is why the loss was invisible at the call site and stayed invisible in tests.

A narrower and more mechanically checkable invariant fell out of the fix and is worth a rule of its own: for a metadata key whose absence is destructive — the slot-owned keys, where a save that omits the key deletes it — every save path must write that key unconditionally. A conditional write is indistinguishable from a delete, and the difference only shows up as data loss on some later read. That one is statically checkable: enumerate the slot-owned key set, then require each save site to assign every member of it.

Visual evidence

Opening the companion chat silently POSTs /api/chat/slots/<slot>/context so the agent knows
which artifact it is looking at. This PR gives that POST's failure its own surface. Both frames
are the REAL built SPA driven by website/scripts/capture-artifact-context-notice.mjs, which
asserts as well as photographs -- it exits non-zero unless the notice renders its own title and
carries no transport text.

The enqueue failed outright -- consequence and remedy, not the transport error:

Artifact detail page showing a dedicated error notice titled "Latest version not shared with the agent", reading "Mention the artifact in your next message so the agent can see it."

The queue answered 429 context_not_queued -- distinct copy, so the capacity case is not
reported as an outright failure:

The same notice under a 429 capacity refusal, titled "Latest version not shared with the agent", reading "Mention the artifact in your next message so the agent can see it." The 429 and generic paths render the SAME body, so this frame pins the refusal PATH rather than distinct copy.

The FIRST injection failed -- the generic title, scoped apart from the resume-freshness wording:

Artifact detail page showing the error notice titled "Couldn't share the artifact with the agent", reading "Mention the artifact in your next message so the agent can see it."

The generic title with the CAPACITY message renders pixel-identical to the frame above -- the notice copy does not vary by cause -- so the capture harness asserts that combination without committing a duplicate frame.

Aggregate persistence budget at the handover union. Per-slot admission (pending_context_budget_room) bounds each queue against its own cap, but a rows-only save unions a DIFFERENT holder's queue onto the same metadata line, and neither side's admission saw the other. _maybe_rotate can only drop MESSAGE lines -- never the metadata one -- so a large enough union evicted real transcript rows. merge_pending_context now keeps entries in delivery order up to HALF the session byte budget, leaving the rest of the file for history.

Disposition for entries that do not fit: they are SHED FROM THE TAIL AND RECORDED by ctxId in a logger.warning naming the budget and the count. That is the difference from the size cap removed earlier in this PR, which skipped non-fitting entries silently so a caller told 200 could not learn its content was gone. The first entry is always kept, so a single oversized row cannot empty the queue. The bound is intrinsic to the union -- it takes no size parameter -- so all three call sites are covered and no caller can opt out.

Before this change both cases wrote to the page's SAVE-error channel, so the banner was titled
Save failed -- telling a user with a dirty draft their work had not been written -- and the
429 was rewritten by friendlyErrText into the tunnel rate-limit string, naming the wrong cause
and promising an automatic retry nothing performs.

Reproduce: cd website && node scripts/capture-artifact-context-notice.mjs

Release notes for the next changelog port

CHANGELOG.md is not writable from this PR — scripts/check_changelog_history.py requires the
file at head to carry ONLY shipped sections (draft_headings refuses an ## [Unreleased]) and
every released section is frozen append-only history. So the two operator-visible changes below
are staged here for the release-porting commit rather than added to the changelog directly.

  • /context can now answer 429 context_not_queued (/note does NOT -- it keeps writing its
    visible line and reports contextSkipped: true). The per-slot queue used to
    evict its oldest entry to make room, so the endpoint always answered 200. It now REFUSES instead,
    because eviction silently destroyed content the API had already acknowledged — the bug class this
    change exists to close. Callers that treated 200 as unconditional must handle 429 and retry.
    Known consequence, bounded: a slot holding entries with no maxAge that never takes another turn
    refuses every later post until the queue-level backstop ages them out, where eviction self-healed
    at once. A staleness horizon IS set here — DEFAULT_CONTEXT_TTL_SECS, seven days.
  • Persisted channel bindings are now trust-checked, and a refusal makes a thread go quiet. This
    rider's regression surface is CHANNEL ROUTING, which is disjoint from the context-durability fix
    it ships with, so it is called out separately. _rehydrate_slot_from_history and
    _apply_recent_session previously adopted a persisted linked_session_key unconditionally; they
    now adopt only a value that names the transcript being hydrated, since that field is
    agent-writable and decides where the slot ROUTES. If a legitimate spelling is ever refused the
    slot stays UNBOUND and answers from its own dashboard session, so a channel thread stops seeing
    replies with only a logger.warning and a SEL chat:adopt_persisted_binding deny event to
    explain it. Two spellings were refused in error during development, both from drift between
    ConversationLog._path and transcript_stems.
  • DEPRECATED: the app-kit "re-post your context on reconnect" recipe. A caller following it now
    DOUBLE-INJECTS, because queued context survives the close it used to be lost to. Nothing
    server-side detects a re-post — there is no idempotency key on /context — so this cannot be
    fixed for external callers from inside the gateway. Only the in-repo caller
    (website/src/pages/ArtifactDetailPage.tsx) is adapted, by holding an in-memory injection claim
    around the POST. External callers should key on their own idempotency marker instead of on the
    endpoint, as docs/app-kit/api-reference.md now states.

Declared riders and accepted risks

Operator note — the persisted-binding gate narrows previously unconditional adoption.
_rehydrate_slot_from_history and _apply_recent_session used to adopt a persisted
linked_session_key on sight; they now adopt only a value that names the transcript being
hydrated, because that field is agent-writable and decides where a slot ROUTES. The
tightening is deliberate, but it has an operational failure mode worth knowing before it
is met in the field: if a legitimate spelling is ever refused, the slot comes back UNBOUND
and answers from its own dashboard-only session, so a channel thread stops seeing replies
with only a logger.warning naming the accepted spellings to explain it. Both outcomes are
now recorded in the Security Event Log (chat:adopt_persisted_binding), so the decision is
auditable rather than inferable from a rotated log line. Two spellings were refused in
error during development, both from drift between ConversationLog._path and
transcript_stems; that agreement is pinned by
test_transcript_naming_is_closed_over_transcript_stems.

Recorded rationale — why the session metadata line, not a per-session sidecar. The
design review asked for this to be written down if the shape stays. A sidecar record with
its own lifecycle would indeed remove the "absence means cleared" coupling, the digest
compare-and-clear retirement, and the byte-budget interaction. It was not chosen because
the queue must be seated and cleared ATOMICALLY WITH the binding it belongs to: a slot's
linked_session_key, its transcript identity and its queue are read on the same hydration
and rewritten on the same save, and update_metadata_if already gives that one
compare-and-set. A sidecar splits it into two writes with no shared commit point, so a
crash between them yields a queue seated against a binding that no longer holds — the same
class of loss this PR closes, reintroduced at a different seam and harder to detect
because each file is individually well-formed. The coupling is real and is the price of
one atomic write; it is pinned by a census test rather than left to convention.

Accepted risk — a queue of no-maxAge entries can wedge a dormant slot. Capacity
is now enforced by REFUSAL rather than by evicting the oldest entry, and the queue is
only drained by a user turn. So a session that never takes another turn keeps its seats
occupied, and later /context and /note posts to it are refused with 429
indefinitely, where the previous FIFO eviction self-healed. This is a deliberate
trade: eviction silently destroyed content the API had already acknowledged with a 200,
which is the class of bug this PR exists to close, and a refusal is at least visible to
the caller. A staleness horizon IS set here: DEFAULT_CONTEXT_TTL_SECS, seven days,
applied inside context_entry_expired — the single expiry chokepoint — and only to entries
carrying a real timestamp, so an unstamped entry keeps its never-expires behaviour.

Consequence — pending_context is slot-owned, so absence means CLEARED. Every save
path must therefore write the key unconditionally, and every hydration path must seat
it, or an ordinary save deletes a copy an earlier close committed. That makes each save
and hydration site load-bearing rather than incidental; the census test
test_the_set_of_metadata_write_sites_is_pinned exists to fail when a new save path is
added without classifying it.

@rnoack1
rnoack1 requested a review from a team as a code owner August 29, 2026 17:45
@rnoack1
rnoack1 requested a review from iamwhatever August 29, 2026 17:45
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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

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

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

BLOCKING -- src/kiro_crew/history.py:416 -- Overflow splitting reorders queued context
if kept and used + cost > budget: excess.append(entry)
Large entry followed by smaller entry -> smaller entry remains on-line ahead of spilled predecessor -> restored context reaches the model out of order.
Anchor: residual/crash-data-loss-corruption
Fix: After the first overflow, spill the entire remaining suffix.

BLOCKING -- src/kiro_crew/history.py:1385 -- Failed quarantine leaves retired context hydratable
cleared = clear_ctx_overflow(key, base, quarantine="always")
Sidecar read and rename failures -> a survivor remains hydratable through reconciliation -> delivered context is restored again after the metadata copy clears.
Anchor: residual/crash-data-loss-corruption
Fix: Treat nonempty cleared.survivors as failed reconciliation and prevent those entries from hydrating.

BLOCKING -- src/kiro_crew/history.py:3415 -- Failed restoration strands surviving context
holding.rename(original)
Pinned-session deletion quarantines context, then a rename-back failure is only logged -> the transcript survives while its pending context remains unreachable.
Anchor: residual/crash-data-loss-corruption
Fix: Check the pinned refusal before quarantine or make restoration failure durable and recoverable.

BLOCKING -- test/test_pending_context_survives_close.py:1171 -- Tests dismantle shared isolation mid-run (origin: validation)
monkeypatch.undo()
Running these tests -> all patches registered on the shared fixture, including autouse isolation, are reverted before later operations -> test behavior becomes host- and order-dependent.
Anchor: no-test-side-effects
Fix: Use pytest.MonkeyPatch.context() for each temporary patch instead of calling undo().

FINDING -- src/kiro_crew/dashboard/dashboard_persistence.py:117 -- local import "from kiro_crew.dashboard.chat_persistence import durable_queued_context" lacks the required circular-import explanation -> Fix: add a # circular import: … comment explaining the cycle.

[BLOCK-MERGE] 5bbc14b
[GPT-REVIEWED] 5bbc14b

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

All four adjudicable findings are empty; the block between the input markers is empty. Four fenced findings (F1–F4) remain, ruled annotate-only. I opened _bounded_context_union, reconcile_ctx_overflow, _restore_quarantined, and the test's monkeypatch.undo() sites in the diff at /home/runner/work/_temp/authentic.patch.

F1_bounded_context_union final branch (patch 3376–3386): once used crosses budget, a later smaller entry can still satisfy used+cost≤budget and stay on-line while its larger predecessor is spilled; the fold (patch 3497–3499) appends spill after on-line with no re-sort, so order is not recovered. But I cannot confirm from opened code that injection ignores injectedAt, nor bound the >½·_SESSION_MAX_BYTES trigger against the 50-entry per-slot cap — the recovery/rarity record is incomplete.

F2reconcile_ctx_overflow read-failure branch (patch 3788–3808): a _quarantine rename OSError appends to survivors and the function only logs and returns; the sidecar stays hydratable and re-injects delivered context. Requires read failure and rename failure; no recovery path in that branch. Record incomplete on rarity — two FS failures is uncommon but not shown to be self-contradicting.

F3_restore_quarantined (patch 3985–3995): rename-back OSError is logged only, so on a pinned-session refusal the holding is stranded off-stem. The bytes persist in the .orphaned-* file but no automatic hydration path reaches them — no confirmed recovery.

F4monkeypatch.undo() (patch 5382, 5419, etc.): reverts every patch on the function-scoped instance, including autouse isolation, mid-test. This fires deterministically on every run of these tests, so no "extreme/rare condition combination" argument exists — the opposite of a FLAG basis.

None supports the complete FLAG record; when torn, UPHOLD-FENCED.

[ADJUDICATION] 5bbc14b02936dca112e7bd8931adf39959999e02 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 5bbc14b02936dca112e7bd8931adf39959999e02
[ADJUDICATION-FENCED] 5bbc14b02936dca112e7bd8931adf39959999e02 fenced=4 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/history.py:416 -- Reorder of queued context on the close-save overflow path is real; cannot confirm injection ignores injectedAt or that the >½-session-max trigger is extreme, so the residual-risk record is incomplete.
UPHOLD-FENCED F2 src/kiro_crew/history.py:1385 -- A read+rename double failure leaves a hydratable survivor that re-injects delivered context with no recovery branch; rarity not shown to be self-contradicting.
UPHOLD-FENCED F3 src/kiro_crew/history.py:3415 -- A logged-only rename-back failure strands pending context off the hydration stem on a pinned refusal with no automatic recovery path.
UPHOLD-FENCED F4 test/test_pending_context_survives_close.py:1171 -- undo() reverting autouse isolation fires deterministically on every run, so no extreme-condition argument for accepting the residual risk exists.
[GPT-ADJUDICATED-FENCED] 5bbc14b02936dca112e7bd8931adf39959999e02

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound root-cause fix, but a disjoint channel-routing trust gate and heavy tail-case machinery ship in one revert unit, and default callers stay lossy.

Watch

  • The persisted-binding trust gate is a separable change riding a durability PR. The description itself calls its blast radius "CHANNEL ROUTING, disjoint from context durability," and its failure mode — "a channel thread stops seeing replies with only a logger.warning" — already occurred twice in development via stem drift. A field regression will bisect to a misleadingly-titled PR, and reverting either change takes the other; the repo rule is one logical change per commit.
    Clears when: the hydration trust gate lands as its own PR, or a maintainer explicitly accepts the coupling.
  • The headline harm survives for every default caller. The motivation condemns "silent loss of data the system acknowledged," yet rider 9 keeps ephemeral: true as the default, so an unflagged /context post is still lost outright on close — the loss is now documented contract rather than fixed. The retention rationale (no unconsented disk writes) is real, but the title promises more than the default delivers.
    Clears when: a maintainer confirms opt-in durability is the intended endpoint contract given the unredacted-content retention trade.
  • The overflow-sidecar lifecycle is large surface for a rare tail. Quarantine/reseat/reconcile, a newly fallible delete_session, and four in-memory buckets with per-ctxId ownership exist chiefly for the 11+-same-transcript-holder union case; a downgrade orphans the sidecar directory silently. It is spec'd and census-pinned, but it is permanent maintenance cost bought for an edge.
    Clears when: a maintainer accepts the sidecar lifecycle as owned surface, or the final-save disposition simplifies to shed-and-log.

[DESIGN-REVIEWED] 5bbc14b

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 5bbc14b02936dca112e7bd8931adf39959999e02 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 is complete. The base-tree defect is confirmed real (in-memory queue, close pops the slot, drain clears before the provider call, FIFO eviction after a 200), the persistence mechanism is reused rather than invented, and the two description-vs-diff gaps are confirmed. Composing the review.

First-Principles-Verdict: CONCERNS

Declared rider 10 (user_messages) ships nowhere in the diff, while the diff's largest real rider — the two-phase drain — is declared nowhere.

Not justified as shipped

  • Item 7 (two-phase drain) — undeclared. _ctx_inflight, commit_drained_context, event-classification tables, _pending_context_gen, a new mid-turn disk save, and a new accepted residual (one duplicate injection) — none of it appears among the twelve declared riders, in a PR that declares riders precisely because reviewers may see only the first 8 KB.
  • Item 5 (Papyrus durability) — undeclared. Rider 8 declares only maxAge: 3600; the diff also flips Papyrus from ephemeral: true to false (patch line 11124), a changed default declared only for the artifact companion.
  • Item 10 (rider 10 phantom). Description: "a new user_messages count on slot state and the slot projection." Grepped user_messages|userMessages across the patch: 1 hit, a test comment (line 11778); the shipped page states "NO RETRY AND NO AUTO-RETRACTION ON A USER TURN" (line 11630) — the diff repudiates the mechanism the description declares.
  • Item 9 (linked_session_key gate) — rides along. The description itself calls its blast radius "disjoint from context durability," and it carries a measured false refusal (legitimate literal-underscore keys now unbind) plus an undeclared audit-or-deny coupling: adoption is refused when the SEL write fails.

What this change ships

Inventory (10 items, capped — the change has more) — 6 justified

Intent: stop the gateway silently destroying background context it acknowledged with 200 — a FIX (defect confirmed on base: state.py:3845 in-memory only, chat_handlers.py:4873 close pops the slot, no hydration site restores the queue; the PR names its failing-on-base test).

  1. Queued context survives tab close and gateway restart — justified
  2. A full queue answers 429 instead of silently evicting the oldest acknowledged entry — justified
  3. Entries with no maxAge now expire after 7 days — justified
  4. Artifact-companion context becomes durable with a 1 h TTL — justified
  5. Papyrus co-author context becomes durable — undeclared (rider 8 declares only the TTL)
  6. contextKey makes a repost after reload a no-op — justified
  7. A turn that dies before reaching the provider re-feeds its drained context — undeclared, non-trivial surface of its own
  8. Over-budget context spills to a sidecar; deleting a session can refuse — justified
  9. A persisted linked_session_key that can't be proven now leaves the slot unbound — rides along (description: "disjoint"), known false refusal
  10. Context-failure notices on the artifact and Papyrus pages (13 catalogs) — justified, but their declared user_messages retraction field is a phantom

Capped at 10: also ships a new 409 resume_metadata_conflict on resume, off-loop metadata reads, a /note budget pre-check, retention of foreign-stamped context, and a distinct prompt frame for restored entries — all undeclared.

Watch

  • Rider 10 declares a field the diff does not contain; a reader of the declaration list will believe a projection field exists that doesn't. Clears when: the description drops rider 10 (and rider 12's retraction claim) or the field actually ships.
  • The two-phase drain is cause-level for the same root defect (loss of acknowledged content between drain and provider), but it is the PR's biggest machinery and invisible in the declaration list. Clears when: it is declared alongside the other twelve, with its one-duplicate residual named.
  • The linked_session_key gate changes channel routing: a legitimate persisted key with a literal _ at the stem boundary bound before and silently unbinds now (the PR's own test measures the refusal). Clears when: the author confirms no supported channel key alphabet can place a literal underscore at that boundary.
  • Papyrus durability flip: an idle co-author session now loses document context after 1 h where its (memory-only) entry previously lived until drain. Clears when: rider 8 declares the ephemeral: false flip, not just the TTL.

[FIRST-PRINCIPLES-REVIEWED] 5bbc14b

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 5bbc14b

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/persist-pending-context-across-close branch from b825b9a to 4cb86c6 Compare August 29, 2026 19:10
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/persist-pending-context-across-close branch from 4cb86c6 to de97fff Compare August 29, 2026 19:36
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/persist-pending-context-across-close branch from de97fff to 69bf44a Compare August 29, 2026 20:25
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/persist-pending-context-across-close branch from 69bf44a to bfbb1f6 Compare August 29, 2026 20:37
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@rnoack1
rnoack1 force-pushed the fix/persist-pending-context-across-close branch from 45020bc to 032d191 Compare August 30, 2026 02:32
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@rnoack1
rnoack1 force-pushed the fix/persist-pending-context-across-close branch from 032d191 to 5960d32 Compare August 30, 2026 03:48
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

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

The base tree confirms the screenshots are PR-added (not on disk), and the capture script only covers the artifact page. I have everything needed: the diff adds two dismissible failure notices (artifact page: two title variants; Papyrus: one), all localized across 13 catalogs, using the established ErrorNotice primitive with documented askAgent opt-outs. No hard swaps, no hedging copy, labels match handlers. But no first-time reader has seen any of it in this lane, and the PR description claims a retraction behavior the shipped code explicitly declines.

UX-Verdict: CONCERNS

Honest, well-layered failure notices — but none has been seen by a cold reader, and the PR body promises an auto-retraction the code deliberately doesn't ship.

Watch

  • Description/behavior mismatch: the PR body (riders 10 & 12) says "The notice retracts on the next USER message" via a new user_messages field — but the shipped code pins the opposite (// NO AUTO-RETRACTION ON A USER TURN, test: "the notice STILL stands") and user_messages appears nowhere in the diff outside one test comment. Anyone approving from the description expects a self-clearing notice; the shipped one persists until dismissed. Once-per-reader but it is the PR's own stated contract — update the body to match the code.
  • PR-body alt text for frames 01/02 quotes the generic body ("so the agent can see it") on the refresh path, which the code renders with the stale body ("sees the latest version") — correct the alt text or re-capture.

Evidence gaps

  • Artifact-page context-failure notice (both titles, both bodies, dismiss control): after-01/02/03-*.png are added by this PR (binary-only in the patch, absent from the base tree) and no blind read ran — push the branch to this repository for the blind-read pass.
  • Papyrus co-author notice ("Couldn't share the paper with the co-author"): no screenshot exists anywhere; the capture script covers only the artifact page.
  • The stale-body variant is verified by nothing: capture-artifact-context-notice.mjs asserts only the shared prefix "Mention the artifact in your next message", so "sees the latest version" is never pinned in pixels or assertion.

Suggestions

  • In capture-artifact-context-notice.mjs, assert the full body per scene ("sees the latest version" for 01/02, "can see it" for 03) instead of the shared prefix, so the two bodies cannot silently collapse into one.
  • Add a Papyrus scene (or frame) for apps.papyrus.workspace.context_notice_title so that surface has any visual evidence at all.

[UX-REVIEWED] 5bbc14b

@bolichen97

Copy link
Copy Markdown
Collaborator

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 #5933 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5933: KEEP. Nearest code-near candidate by shared files and by both depending on update_metadata_if merge behaviour, but materially different scope and no measured interaction. Files: src/kiro_crew/dashboard/chat_persistence.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #2783. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. Same attribute and same authorization seam, with a real semantic interaction (a refused app-slot binding is precisely the case 6813's queue filter and held-entry parking react to). Coordinate the two rather than resolving textually. Files: src/kiro_crew/dashboard/state.py, src/kiro_crew/dashboard/chat_persistence.py.
  • This PR is OVERLAPPING with PR #3248. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. Ancestor feature rather than competing work; nothing about it makes 6813 redundant. Files: src/kiro_crew/dashboard/chat_handlers.py.
  • This PR is OVERLAPPING with PR #4623. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. A direct same-line conflict on a guard both PRs widen for unrelated reasons; the composed condition should be written once rather than reconstructed by whoever rebases second. Files: src/kiro_crew/dashboard/chat_persistence.py.
  • This PR is OVERLAPPING with PR #6823. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. Independent goals that collide on the same /note handler block and the same api-reference section, and 6813 invalidates the durability premise 6823 records for visibleOnly. Land order and a single reconciled doc paragraph need to be agreed rather than resolved as a mechanical conflict. Files: src/kiro_crew/dashboard/chat_handlers.py, docs/app-kit/api-reference.md.
  • This PR is OVERLAPPING with PR #6831. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. Complementary halves of the same endpoint with no behavioural contradiction; only the adjacent api-reference text and the shared handler body need reconciling at merge time. Files: docs/app-kit/api-reference.md, src/kiro_crew/dashboard/chat_handlers.py.
  • This PR is OVERLAPPING with PR #6915. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. The merged refactor is the current home of the eviction policy 6813 removes; deleting the now-unused helper rather than leaving a dead body carrying the defective policy is the right disposition, and no other caller exists in main. Files: src/kiro_crew/dashboard/slot_buffers.py.
  • This PR is OVERLAPPING with PR #7212. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. 7212's deferred-key set is derived from the frozenset 6813 extends, so landing both silently decides how pending_context behaves on a rows-only save; the census tests 6813 adds should be made to cover that third class explicitly. Files: src/kiro_crew/history.py, src/kiro_crew/dashboard/chat_persistence.py.
  • PR #7163 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7163: REBASE. Adjacent work on the same deferred-note/pending-context structures with no conflicting semantics; 6813's budget accounting already tolerates a context-less entry. Files: src/kiro_crew/dashboard/state.py, src/kiro_crew/dashboard/slot_buffers.py.

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

@bolichen97

Copy link
Copy Markdown
Collaborator

@rnoack1 A repo-wide audit flags five open PRs that overlap this one. Nothing on main or in any open PR implements this durability fix, so the ask is coordination, not a close. Audited at f575e7c; the head has moved since, but all shared files below are still in the current diff.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

Undrained /context and /note entries were in-memory only, so closing a tab
discarded content the API had acknowledged; they now round-trip via session metadata.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants