docs(session-storage): record the read-only-resume gap in the revival guard

[chenmingwei23]

What is the problem?

docs/system-specs/modules/session-storage.md records the residual race in
move_to_trash's revival guard as microseconds wide. It is only that wide for a resume
that WRITES. A resume that only reads the old transcript is not detected at all, and its
window is the whole move loop.

Four spans in the doc say or imply the loop catches a resume without qualifying which
kind:

• "The move loop then closes most of what the re-read cannot."
• "The window is therefore the gap between one file's stat and its rename rather than
  the whole loop."
• "The loop's mtime check leaves that one session in place and the rest of the batch
  proceeds."
• the Known Limitations bullet, which is the one a triager reads.

Why this issue matters to the user

Not to an end user directly - to whoever reads this spec to decide whether the gap is
already handled. Two independent triage passes on #7118 both concluded the residual was
already documented and routed the issue on that basis. It is not documented: what is
documented is a different, far narrower race that happens to sit in the same paragraph.
An incomplete Known Limitations entry is worse than an absent one, because it converts
"nobody has looked at this" into "someone looked and accepted it".

How our fix solves it

Docs only - no behaviour change.

The Known Limitations entry is split in two. The existing bullet is scoped to a resume
that writes and otherwise left alone. A second bullet records the read-only case: the
sequence that produces it (files certified reclaimable, session resumed, resume reads the
old transcript to rebuild history, the turn that follows recorded under a newly mapped
SID without rewriting that transcript), why the mtime check cannot see it (every file
still carries its original mtime, so all of them pass the validation-instant check), and
why it is not simply fixed by moving the index re-read inside the loop.

That last part is measured rather than asserted, because the reason the fix is deferred
is a cost: refresh is _build_index, which is a file read plus a full json parse, so
it has a floor of about 0.26 ms however small the map. At the _MAX_SELECTION cap of
200,000 units that is ~56 s at the floor and hours against a realistic map size, and it
would take SessionMap._MAP_LOCK once per unit while the reclaim lock is held. The
affordable form needs the caller to memoize its index on a change token, which is a
contract change to refresh rather than an adjustment to the loop - so the entry says
that, instead of leaving the next reader to re-derive it.

The three prose spans above get a scoping clause each and a pointer to Known Limitations.
Correcting only the bullet would have left the body of the same document contradicting it.

What tests we did

No test ships with this. The gap was reproduced first - a regression test in
TestASessionResumedWhileStagingIsLeftAlone where the second session becomes mapped
mid-loop while writing nothing fails on kirocrew/main at 23c99c0f1 with
AssertionError: assert () == ('bbbb2222',), confirming the live session is staged. That
test cannot land here: with no fix it is permanently red, so it is attached to #7118 for
whoever ships the fix rather than committed.

The measurement quoted in the new bullet is the median of 5 runs of _build_index() plus
the two set unions the pre-loop code already performs, at map sizes 100 / 1,000 / 10,000
/ 100,000, projected to the cap. Full table on #7118.

Verified no CI gate reads this file: no test references session-storage.md, Screenshot
Evidence scopes to user-visible frontend surfaces, and First Principles skips a docs-only
diff.

Any other suggestions on the work

#7118 itself should stay open and stay parked. Of the two shapes it proposed, the
per-unit refresh is ruled out by the measurement above and the post-loop-refresh-plus-
rollback shape needs a manifest tombstone honoured by _read_manifest,
_summarize_manifest, _restore_locked, _manifest_rels, _listed_bytes,
staged_targets and _empty_trash_locked - every one of which is inside #7011's
unmerged diff. The third shape (caller-memoized refresh) is a contract change and wants a
maintainer's call.

One thing found while measuring, worth its own look eventually: _build_index() reads the
session map FILE, but SessionMap._save() debounces by _FLUSH_DEBOUNCE_SECS = 0.05. So
any file-based refresh - including the pre-loop one already on main - is blind to a resume
for up to ~50 ms after it happens. That bounds how well any of the three shapes can do
without consulting the live in-process map.

Refs #7118

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Verified against src/kiro_crew/session_storage.py: refresh runs once before the move loop (line 1698) and the only in-loop guard is the per-file mtime > validated_at check (line 1795), so a read-only resume genuinely passes undetected for the whole loop — the corrected doc matches the code, and all four prose spans are updated consistently rather than just the Known Limitations bullet.

Design-Verdict: PASS

Accurate, code-verified correction of a misleading Known Limitations entry, with the deferral rationale measured and recorded — right scope, right layer, fully reversible.

[DESIGN-REVIEWED] 556d4a6

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 556d4a6

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 556d4a6

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

[buluoray]

Reviewed at 556d4a6ed. 0 blocking / 0 yellow / 1 blue — approving.

This is the kind of spec correction worth landing promptly: the old prose implied the move loop's mtime check catches any resumed session, which had already led two triage passes to wrongly conclude the gap was documented.

Verified the new claims against implementation rather than reading them: the loop stats each source for the manifest and that same stat carries mtime; if mtime > validated_at is the only revival signal, with validated_at = time.time() taken before the scan; woke sessions are rolled back and appended to TrashBatch.revived; move_to_trash re-reads via refresh inside the mutation lock and unions the active sets; refresh=_build_index does SessionMap().mapped_sids_by_key(), i.e. a file read plus a full json.loads; _MAX_SELECTION = 200_000. So the read-only-resume blindness is a direct provable consequence of the guard depending entirely on a write bumping mtime — matching #7118.

Also checked the deletions, since a docs PR removing accurate content is the failure mode: all 17 removed lines are overclaims ("closes most of what the re-read cannot", unscoped window language) replaced by correctly-scoped versions. docs-lint.sh on a worktree of the head exits 0. CHANGELOG untouched.

Blue: the "it would take SessionMap._MAP_LOCK once per unit" line is accurate as written, but note SessionMap._load is deliberately not @_guarded, so the unbounded json read itself is not held under the lock. The doc does not claim it is — precision note for a future reader only.