fix(snapshot): copy notification records as bytes, not through a locale decode

[chenmingwei23]

1. What is the problem?

snapshot._merge_notifications appends the snapshot's notification records into the live notifications.jsonl verbatim. Both handles were text mode:

with open(dst_path) as f:                                # destination scan
    for line in f:
        try:
            existing.add(json.loads(line).get("ts") or line.strip())
        except (ValueError, TypeError):
            pass
with open(dst_path, "a") as out, open(src_path) as f:    # the copy
    for line in f:
        try:
            key = json.loads(line).get("ts") or line.strip()
            ...

So a verbatim byte copy was running through a locale decode followed by a locale encode, with universal-newline translation on top. Neither half is byte-exact, and the issue's own framing -- an "encoding-validation gap" -- names a symptom rather than the defect.

Read this before prescribing encoding="utf-8"

The obvious remedy is an explicit encoding= on both open() calls. It does not fix this. Two of the measured failures are locale-INDEPENDENT and fire on a pure UTF-8 host with fully valid UTF-8 input, because newline= is a separate axis from encoding=. Measured on 2d75835f3, UTF-8 locale, PYTHONUTF8=1:

A valid record is silently and permanently lost. A record containing a bare carriage return:

src  in : b'{"ts":"2026-03-04T00:00:00Z","msg":"a\rb"}\n'
printed : Notifications imported: 0        <-- reported as success
dst out : (unchanged -- the record is gone)

Universal newlines split the record in two, both halves then failed json.loads, and except (ValueError, TypeError): pass swallowed both. No encoding was involved.

A valid record loses a byte. A record terminated with a carriage return plus line feed:

src  in : b'{"ts":"2026-01-05T00:00:00Z"}\r\n'
appended: b'{"ts":"2026-01-05T00:00:00Z"}\n'   <-- one byte shorter than the source

The remaining failures are locale-dependent, and the locale decides which one you get. for line in f decodes OUTSIDE the try, so UnicodeDecodeError -- a ValueError subclass -- escaped except (ValueError, TypeError) because the ITERATOR raised it, not json.loads. The escaping traceback's innermost frame is <frozen codecs>:322.

input	locale	measured
invalid UTF-8 in the source	utf-8	restore aborts with a traceback; nothing appended
invalid UTF-8 in the LIVE file	utf-8	aborts at the destination scan, before any append
invalid UTF-8 in the source	ISO-8859-1	merge SUCCEEDS, bytes land verbatim, live file is no longer valid UTF-8
unterminated final record either side	any	two records glued into one line that parses as neither

On the single-byte-locale row: latin-1 and cp1252 are byte-BIJECTIONS on decode-then-encode. All 256 byte values were checked; zero differ for either codec, and cp1252 rejects exactly five (0x81 0x8D 0x8F 0x90 0x9D) and round-trips the other 251 identically. So that path does not garble the bytes -- it faithfully delivers invalid UTF-8 into a file whose reader demands UTF-8, which is the harm.

2. Why this issue matters to the user

The live file's loader decodes the whole file inside one try whose except Exception returns [], so one bad byte costs every record, not the bad row. Measured with KIROCREW_HOME on a temp dir:

5 valid records + 1 invalid-UTF-8 record on disk
  _load_notifications()    -> 0 rows        (not 5)
  _rewrite_notifications() -> file is 0 bytes; all 5 valid records gone

Any delete, acknowledge or clear triggers that rewrite. So the user's outcome is a restore that reported success, then total loss of notification history the next time they dismiss a notification. And the two locale-independent failures lose records on the default configuration with no bad bytes anywhere.

3. How our fix solves it

Symptom to root cause: records were lost or garbled -> because the appended bytes were not the source bytes -> because a verbatim copy was being decoded, re-encoded and newline-translated -> because both handles were text mode. So the fix is to stop being in text mode.

Both reads now go through jsonl_util.strict_raw_records on BINARY handles. That module's own docstring names this caller and says it was left unconverted pending exactly this contract, and strict_raw_records says "use where the record must survive byte-for-byte: a reader that copies records into another file cannot go through a lossy decode and re-encode". Records come back with their terminators, undecoded and byte-exact.

The write-side contract the issue asked for, stated up front and now on the function itself:

• Boundary -- the universal-newline set, which is what the text-mode iteration split on, with the pending-carriage-return half-pair handled inside the framing layer.
• Encoding -- validated by decoding, but the decode's RESULT is used only for the dedupe key; what gets appended is always the original bytes. Validating by decoding and writing the decoded form back is precisely the round trip being removed.
• Framing -- every appended record ends with a terminator, and an unterminated final record already in the destination gains one before anything is appended after it.
• Key type -- well-defined and hashable for every record shape. A non-object record no longer raises AttributeError off .get, and a list or dict ts no longer raises TypeError on set insert. A ts is used whenever it is truthy AND hashable, under a kind tag deliberately coarser than its Python type, because two equalities are in play at once: True == 1 and they hash equal, so an untagged key DELETES a ts: true row as a duplicate of a ts: 1 row; 1 == 1.0 and they hash equal too, so tagging with type(ts).__name__ SPLITS an integer-versus-float spelling of one row and persists a duplicate. One tag covers both -- int and float share "num" so they deduplicate exactly as the predecessor's bare-value key did, while bool is its own tag and is tested FIRST because it is a subclass of int. A record with NO usable ts yields no key and is never deduplicated: the predecessor fell back to line.strip(), which is content used as identity, and for a row that does not parse that is not identity -- two different records whose stripped bytes coincide collapse and the loser is deleted.
• Failure posture -- abort, never skip, because the output feeds a durable append and a skipped record is a deleted one. Abort means RAISE, not warn: apply_import_zip appends notifications (merged) to its summary unconditionally and the dashboard handler answers ok: True with a SEL outcome="ok", and neither sees stdout, so a printed warning alone would tell an API caller that an import which left records behind had finished. The print stays so a CLI operator reads the reason before the traceback. Both scans are no-ops on refusal: the destination is not opened for append until its scan completes, and the ENTIRE source is validated before it is opened, because an identity-less row cannot be deduplicated so any prefix left by an aborted copy is one a retry would append again. A failure DURING the copy is the residual case -- the source changed between the two passes -- and only there does a prefix survive, since rolling it back would be a second unvalidated write.
• Bound -- a per-record cap that aborts. It arrives with the reader.

The destination scan is converted too, and that is not scope creep. The issue's own acceptance criterion "destination-scan failure must be a true no-op" is about that site, even though neither the issue body nor its triage comment names it. It had the same defect: an invalid record already in the LIVE file aborted the merge with a traceback before the copy loop was reached.

The bound is in scope for the same reason it could not be left out. snapshot.py held exactly two for <var> in <handle>: loops and both were in this function, so converting them removes the file's last instance of the shape -- at which point test_the_scanner_actually_detects_the_pattern requires it to leave _DEFERRED_READERS, since that test asserts every excused file still contains the shape. And the byte-exactness fix requires a binary framing reader, which is strict_raw_records, whose cap is a defaulted keyword. Refs #6345 is here so that issue's owner sees a part landed; this does not claim to close it.

4. What tests we did

Twenty-one new tests in test/test_snapshot.py::TestNotificationMergeWriteSideContract, one per contract property. Every fixture is real BYTES written to a real file: a synthesized UnicodeDecodeError would route the merge down a healthy path and prove nothing, because the whole defect was that the decode happened at for line in f, outside the try, so the real failure never took the branch a fake exception takes.

Mutation-verified one enforcement SITE at a time, twenty-four mutations, each reverted with the file asserted restored byte-for-byte:

property	how the mutation failed
encoding, source side	Failed: DID NOT RAISE UndecodableRecord
encoding, destination side	Failed: DID NOT RAISE UndecodableRecord
a destination-scan failure must not fall through to the copy	Failed: DID NOT RAISE UndecodableRecord
abort, not skip, on an undecodable source record	Failed: DID NOT RAISE UndecodableRecord
the failure reaches the caller, both sites	Failed: DID NOT RAISE UnreadableRecord
byte-exactness against newline translation	byte equality, both carriage-return tests
framing, destination terminator	At index 42 diff: b'{' != b'\n' -- the records glued
framing, source terminator	byte equality
key type, non-object row	Failed: the merge aborted ... AttributeError("'list' object has no attribute 'get'")
key type, unhashable ts	Failed: ... TypeError("unhashable type: 'list'")
the per-record cap	Failed: DID NOT RAISE OversizedRecord
a numeric ts still deduplicates	AssertionError: a numeric ts stopped deduplicating
an int and an equal float ts deduplicate	AssertionError: an int and an equal float ts stopped deduplicating
ts: true does not collide with ts: 1	AssertionError: a boolean ts collided with 1
bool is not swallowed by the numeric arm	AssertionError: a boolean ts collided with 1
a split record's fragment is not skipped	AssertionError: a record fragment was skipped as a duplicate
an identity-less row is never skipped	AssertionError: the declared re-append
both existing.add guards keep None out of the seen-set	AssertionError: an identity-less row was dropped
the source pre-validation pass	AssertionError: first: a prefix was appended
the pre-pass must call the operation that DECODES	AssertionError: first: a prefix was appended
the success line prints exactly once	assert 2 == 1, with both copies in the message
an archive-derived path is escaped before printing, pre-validation site	AssertionError: a raw escape reached the terminal
the same, at the residual copy-failure site	AssertionError: a raw escape reached the terminal

Every red is on pytest's assertion channel -- AssertionError, or Failed from DID NOT RAISE or from a _merge_must_not_abort helper that names the property instead of surfacing an incidental exception.

Two of those deserve calling out because they are not obvious. The success-line mutation exists because a restructure re-emitted that print and no linter, formatter or type checker sees a doubled print -- and every other assertion here spells the check "Notifications imported:" in out, which is structurally blind to duplication whatever suffix it carries, so the test counts instead of testing membership. And the pre-validation pass has two mutations because removing the pass and keeping the pass while not calling the key function fail identically: strict_raw_records does not decode, so draining the reader proves nothing about encoding.

Suites, one named file per invocation, serially (-n 0), with PYTHONPATH pinned to the worktree so main's installed copy could not be the thing under test:

• test/test_snapshot.py -- 92 passed (71 pre-existing, including the two pre-existing notification-merge tests, which pass unchanged)
• test/test_jsonl_util.py -- 40 passed, including the audit scanner with snapshot.py removed from the excused set
• test/test_portability.py -- 51 passed (the second caller of this function)

Gates: isort, flake8, mypy, scripts/check_black_formatting.py, scripts/check_subprocess_encoding.py, scripts/check_lockdown_before_publish.py, scripts/check_testpaths_coverage.py, scripts/check_loop_bound_locks.py -- all clean. Mergeability computed rather than polled: git merge-tree --write-tree kirocrew/main HEAD exits 0, and main's delta since the merge-base overlaps none of these four files.

5. Any other suggestions on the work

A bundle chooses its own inner root, so the two SOURCE-path prints escape it. _safe_name exists in this module for exactly that -- its docstring names archive root directories, and nineteen sites already use it -- and the prints this change adds were bypassing it, so a crafted root could move the cursor and overwrite lines right above the prompt where the operator decides whether to trust the restore. Both source prints now wrap the path. The destination print deliberately does not: that path is the live data home, chosen by the operator, not a name out of an archive.

The exception text is deliberately left unwrapped, and the invariant is stated on the code because it is what makes the wrapper unnecessary rather than forgotten: both types the arm catches already render an embedded path with repr-style escaping -- OSError.__str__ for its filename, and jsonl_util via {path!r}. Measured: a control character in a directory name reaches neither exception's str() raw. Wrapping it as well was over-delivery, and no mutation of it could redden.

The blocked label is stale and can be dropped. It was applied because strict_raw_records and _DEFERRED_READERS did not exist on main. Both shipped in #7651, merged 2026-09-02 as 4c288169029ddea1f0522d90243fe39e029cfa7c, and both were read on main before this change was written.

One correction to the issue's triage comment, because it bears on the remedy. That comment identifies the silent path as one where "the destination gets different bytes than the source had". Measured, that is not the mechanism: under a single-byte locale the copy is byte-faithful, and the bytes that really do change change from universal-newline translation, which is locale-independent. Its headline conclusion -- that the non-byte-exact round trip is the root defect rather than an encoding-validation gap -- is right, and stronger than its own reasoning.

Two residues, both filed rather than folded in. #8181: both callers fall back to shutil.copy2 when the live file does not exist, which is byte-exact and validates nothing, so it delivers the same total-loss outcome on every locale including UTF-8 on a fresh install. Not in this PR because copy2 never decoded anything, so leaving text mode does not touch it -- closing it means ADDING a validation pass rather than converting a pipeline. #8217: _merge_crons warns and returns on three refusal paths while portability.py appends crons (merged) unconditionally, so a refused cron merge is reported as merged -- the same summary-honesty defect this PR fixes for notifications, in the neighbouring component, and its fix needs a judgement about whether one refused component should abort a multi-component restore.

One inherited quirk, declared rather than silently carried. ts: true and ts: 1 are equal and hash equal in Python; the kind tag separates them, but the underlying identity is the language's and is inherited from the code being replaced. It is not a regression this PR introduces and not among the acceptance criteria.

Trailer: Closes #7771 rather than a bare Refs, because all four acceptance criteria are met by this diff and both residues have their own tracked owners.

Scope: one finding acknowledged and tracked rather than fixed here

A review round raised that identity-less notification rows can evict live history. The
finding is real and half of it is fixed in this change: a ts-less row that PARSES is now
deduplicated on its raw unstripped bytes, which restores the predecessor's idempotence for a
re-run without restoring the deletion it caused. Stripping made two distinct byte sequences
share a key; raw bytes cannot, because byte-equal records are the same record.

The remaining half is deliberately not fixed here, and is tracked in #8313:

• A record that does not PARSE gets no key by design, so it appends on every merge. That is
  correct, not a shortcoming: jsonl_util frames on the universal-newline set, so a record
  containing a bare carriage return is split into fragments, and those fragments are exactly
  the unparseable records. Giving them a content key lets one collide with a crash-truncated
  row and be deleted. The docstring records that framing on newline alone was considered and
  rejected for the mirror-image reason, so this is a reviewed tradeoff rather than an
  oversight.
• Genuinely distinct rows defeat any per-record identity, and dashboard/state.py's trim
  keeps the newest rows while deliberately retaining unparseable ones -- which is what turns
  an append into an eviction. That is a second owner's file.
• Whether the merge should refuse a malformed or non-object row outright is a behaviour
  change, not a bug fix: main imports them, and refusing would make one bad row cost an
  entire snapshot restore under this change's validate-whole-source-first posture.

The vector is pre-existing: main keys a ts-less row by line.strip(), which collapses
only identical rows, so it appends N rows for N distinct rows exactly as this branch does.
Nothing here is a regression from this change.

Two reviewers, the same lines, different questions

_notification_key's raw-key return was examined twice with opposite outcomes, and both
readings are honest. Recording it so nobody has to reconstruct why.

Opus 4.8 considered the append's terminator handling and dropped it. Its question was
whether adding a terminator to an unterminated final record HARMS the destination, and the
answer is no: terminating it is correct hygiene, the operation is idempotent, no record is
lost, and nothing becomes unloadable.

GPT 5.6 asked a different question -- whether the added terminator changes the row's
DEDUPLICATION KEY -- and found that it does. An unterminated parsed row without ts was
keyed on its arriving bytes while the append wrote it with \n appended, so a second import
compared the source's unterminated bytes against the terminated row the first import itself
wrote, missed, and appended a second copy.

Reproduced before the fix: two copies after two imports, with Notifications imported: 1
printed both times. Fixed by normalizing the raw key to the bytes that LAND rather than the
bytes that arrive, using the same predicate as the write so the two cannot drift.

The direction of that normalization is load-bearing. Adding the terminator the writer
adds is deterministic and merges only records that land identically. Normalizing the other
way -- rstrip -- would be the predecessor's line.strip() under a new name, mapping X\r
and X onto one key, which is exactly the fragment / crash-truncated-row pair this change
exists to keep apart. A mutation test pins the direction, not merely the presence, because
substituting rstrip left the whole suite green until that test was added.

The general form worth keeping: an earlier "no findings" is not evidence of absence. It is
evidence about the question that reviewer asked.

Pattern harvest

Rule candidate: review-prompt

Pattern: a loop that iterates a file handle in TEXT mode while its body copies the line into another file. The decode sits on the for statement, so it is outside any try in the body, and UnicodeDecodeError being a ValueError makes an except ValueError look like it covers a case it cannot reach. Independently, text mode applies universal-newline translation, so the copy is not byte-exact even when every byte is valid UTF-8. Two questions worth asking of any such loop: does an except ValueError in the body actually catch the decode, and is the handle text mode when the contract is a verbatim copy.

Second candidate, from this PR's own review history: when a change adds a discriminator to a dedupe key, ask what the discriminator JOINS as well as what it SPLITS. Two revisions here each fixed one equality class and broke the other, because True == 1 and 1 == 1.0 both hold and hash equal.

Not a semgrep rule: the shape is easy to match but the judgement -- whether a site's contract is verbatim-copy or consume -- is not, so a syntactic rule would be mostly false positives on consumer reads.

Closes #7771
Refs #6345
Refs #8181
Refs #8217

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

All the load-bearing claims check out: the raise propagates to a caught, audited failure at both callers (restore_main catches UnreadableRecord and returns 1; the dashboard handler's except Exception returns a 500 with SEL outcome="error", so the old "reported success while records were left behind" path is genuinely closed), the shared strict_raw_records reader was pre-reserved for exactly this caller, the 128 MiB cap is a memory bound no real notification record approaches, and the acknowledged residues (copy2 fallback, cron summary honesty, identity-less eviction) are filed rather than folded in. The abort posture does mean one bad byte in a snapshot's notifications now fails the whole dashboard import after earlier components applied — but that same input already aborted with a traceback on a UTF-8 host before this change, so it is a pre-existing non-transactionality made deterministic and audited, not a regression, and the author explicitly tracked the abort-vs-skip judgement in #8217.

Design-Verdict: PASS

Root-cause fix at the right seam: binary framing via the shared reader that was already reserved for this caller, with an honest raise-not-warn failure posture.

Suggestions

• When _merge_notifications raises out of apply_import_zip, the partial summary (memory/crons/config already applied) is discarded and the dashboard reports an opaque 500 — attaching that partial summary to the failure belongs with the snapshot: a refused cron merge is reported to the import caller as a successful one #8217 judgement; make sure that issue covers it.

[DESIGN-REVIEWED] 576a636

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

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

First-Principles-Verdict: PASS

A documented-deferred conversion done at cause level — text mode itself removed, using the reader built for it, with zero unfixed siblings.

What this change ships

Intent: stop the snapshot notification merge losing or corrupting records by copying them as bytes instead of through a locale decode. FIX.

1. Records now land byte-exact; a bare-\r record is no longer silently lost — justified (measured data loss on default config)
2. Invalid UTF-8 on either side aborts the merge instead of corrupting the live file — justified
3. That refusal exits the CLI with ❌ + code 1 instead of a traceback — justified
4. Destination scan converted; its failure writes nothing — justified (issue's own no-op criterion)
5. Whole source validated before any append, so a refusal leaves no retry-duplicating prefix — justified
6. An unterminated final live record gains a terminator before append — justified (glued-records loss)
7. New dedupe key: kind-tagged ts, raw-bytes fallback; odd shapes no longer abort — justified, declared
8. Over-cap record aborts the restore — justified (mandated by the _ALLOWED_UNBOUNDED_FILES scanner gate)
9. Archive-derived paths escaped via _safe_name in the new prints — justified (external-content boundary)
10. Scanner's _DEFERRED_READERS emptied, closing the recorded debt — justified

Checks run: strict_raw_records is the pre-existing mechanism, not a duplicate — jsonl_util.py:389's docstring reserved exactly this caller. Siblings: grepped def _merge_ in snapshot.py — 3 merges; _merge_crons (snapshot.py:2350) re-serializes via json.dumps with explicit UTF-8, so 0 byte-copy siblings remain. _NOTIFICATION_RECORD_CAP = RECORD_CAP follows 2 existing precedents (members.py:68, session_digest.py:28), not a second spelling. Both callers of _merge_notifications (2: snapshot.py:3893, portability.py:564) have a boundary that converts the new raise into a reported failure — the new restore_main arm and the dashboard handler's existing except Exception (dashboard/handlers/portability.py:143). No zero-consumer surface: everything added is module-private and consumed in-function.

[FIRST-PRINCIPLES-REVIEWED] 576a636

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/snapshot.py:2402 -- “Only an unhashable ts … falls through” contradicts the raw-key fallback for falsy values -> Fix: document that falsy and unhashable values use the raw key.
[GPT-REVIEWED] 576a636

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The candidate keys on preserved-not-introduced behavior. The predecessor keyed dedup on ts (json.loads(line).get("ts") or line.strip()), and the new _notification_key deliberately reproduces that — the docstring defends keying on ts as intentional. Collapsing two records that share a ts is the pre-existing dedup semantics, not a regression the changed lines introduce.

The candidate also fails input-reachability: notification ts is datetime.now(tz=timezone.utc).isoformat() (notifications/bus.py:398), microsecond-resolution — the candidate itself concedes it "could not confirm whether notification ts values are guaranteed unique per record in practice," so (a) resolves to a "could," which the rules require me to drop. No survivor at 80+, and no grounded self-originated finding in this heavily-tested diff.

No findings.

[OPUS-REVIEWED] 576a636

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

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

[chenmingwei23]

Confirmed and fixed in d9ddc6030. This was a real regression introduced by the first revision of this PR, not a pre-existing one, and the reasoning is worth stating because it inverts what the change was supposed to do.

Reading the callers rather than the function: portability.apply_import_zip has no try around the merge call, appends notifications (merged) to its summary unconditionally, and dashboard/handlers/portability.py:142 answers {"ok": True, "summary": ...} with a SEL outcome="ok". Its except Exception at :143 is exactly what the pre-fix UnicodeDecodeError reached. So warn-and-return did not merely fail to add a signal -- it REMOVED one the buggy code still had, and it did so in the same defect class this PR exists to close: a partial durable write reported as a complete one.

Both handlers now raise after printing. The print stays so a CLI operator reads a named reason before the traceback; the raise is what apply_import_zip and the HTTP handler need. Deliberately unlike _merge_crons, which warns and returns: a refused cron merge writes nothing and skips one component, whereas this one may already have appended a prefix, so the caller has to learn the write is incomplete.

Everything the fix guarantees is unchanged: a destination-scan failure is still a true no-op (the destination is not opened for append until that scan completes), and a source-scan failure still leaves its prefix in place with a re-run adding no duplicate, because existing is rebuilt from the destination.

Two mutations pin it -- turning either raise back into return reddens with Failed: DID NOT RAISE across 2 and 3 target tests respectively -- and the four tests whose subject changed from "warns" to "raises" were rewritten to assert the propagation rather than adjusted to keep passing. A new test, test_the_failure_reaches_the_caller_and_is_not_only_printed, pins the caller contract separately from the byte-level assertions. All thirteen mutations were re-run after the restructure, not just the two it added.

[chenmingwei23]

Confirmed and fixed in 73dd0b607. The duplicated print was real and it was mine: the edit that added the raising posture matched a block ending at the old return and re-emitted the trailing success line, so the pre-existing one survived below it. grep -c "Notifications imported" src/kiro_crew/snapshot.py now returns 1.

The part worth recording is why nothing local caught it. isort, black, flake8, mypy and every repo gate pass on a doubled print. And all thirteen of my own assertions spell the check as "Notifications imported:" in out, which is satisfied just as well by two copies as by one -- a membership test is structurally blind to a duplicate. So this was invisible to the entire local gate set by construction, not by oversight in running it.

There is now a count assertion rather than a membership one, test_the_success_line_is_printed_exactly_once, and re-adding the duplicate reddens it with assert 2 == 1 and both copies in the failure message. That is the fourteenth mutation.

Also confirmed on the same head: test/test_snapshot.py 84 passed, test/test_jsonl_util.py 40, test/test_portability.py 51, one named file per invocation; isort, black at the pinned --target-version py310, flake8, mypy, and the lockdown and loop-bound-locks gates all clean.

Thank you for verifying the description's claims individually rather than the summary -- the cap-alias sibling count and the _merge_memory/_merge_crons non-siblinghood in particular are exactly the checks that would have caught round 1's regression earlier, since that regression came from treating _merge_crons as a transferable precedent when its caller's contract differs.

[chenmingwei23]

Confirmed and fixed in c9a763c5d. This one is sharper than a gap: it was a regression against the PREDECESSOR, and my guard caused it.

The old key was json.loads(line).get("ts") or line.strip(), which keyed a numeric ts on the number. So two rows carrying the same numeric ts with different bytes -- one normalised, one not -- used to deduplicate. My isinstance(ts, str) and ts sent them to the raw-bytes key instead, which persists a duplicate: the same loss class this PR exists to close, introduced by a guard wider than the hazard it was aimed at. The hazard is unhashability, not non-str-ness. The predicate is now truthy-and-hashable exactly as you prescribed, with hash(ts) as the test and only an unhashable ts falling through to the raw form.

test_a_numeric_ts_still_deduplicates_on_its_value pins it with two spellings of one row -- {"ts":1767225600,"msg":"..."} in the live file and a key-reordered variant in the snapshot -- asserting imported: 0 and the destination unchanged. Reverting the predicate to isinstance(ts, str) reddens it with AssertionError: a numeric ts stopped deduplicating.

One declared rider from the same fix, because widening to any hashable ts exposes a second property rather than the same one: the key now also carries the ts's TYPE NAME. True == 1 in Python and the two hash equal, so a row with ts: true and a row with ts: 1 would otherwise be one set member and the second would be discarded as a duplicate -- a deleted record from a ts that is merely a different type. test_a_boolean_ts_does_not_collide_with_the_number_one pins it, and dropping the type name from the key reddens it on byte equality with the second record missing. Both halves are in the description under Key type, not left to be discovered in the diff.

Same head: 86 / 40 / 51 across test_snapshot.py, test_jsonl_util.py, test_portability.py, one named file per invocation; isort, black at the pinned --target-version py310, flake8, mypy and the lockdown gate clean. Sixteen mutations total, each reverted with the file verified restored byte-for-byte.

[chenmingwei23]

Confirmed and fixed in b52554040, but with a NARROWER remedy than you proposed, and the difference is the point.

Your finding is right and it was my own rider that caused it. I added type(ts).__name__ to the key one round ago to stop ts: true colliding with ts: 1, since True == 1 in Python. You spotted what the tag also does: 1 and 1.0 are equally equal and hash equal, so tagging the type turns a row written as an integer on one side and a float on the other into two records and persists a duplicate. Measured -- the predecessor's bare-value key collapses {1, 1.0, True} to a single member and keeps "1" separate, while the tagged key gives four.

So the rider traded an exotic collision for a common one. An integer-versus-float timestamp is an ordinary serializer artefact; a boolean timestamp is not.

Your suggested fix -- one numeric tag for integers and floats, booleans kept distinct -- would work. I did not take it, because it keeps my addition and adds a taxonomy to compensate for it. Instead the addition is deleted: the ts goes into the key as itself, so the numeric behaviour is exactly the predecessor's, which is what your ROUND 3 remedy text asked for in the first place ("preserve any truthy, hashable ts; use the raw key only for unhashable values"). I over-delivered on that round and this round is the correction.

That leaves the ts: true / ts: 1 identity in place. Stating it plainly rather than leaving it to be found: it is Python's own equality, it is inherited unchanged from the code being replaced, it is not a regression this PR introduces, and it is not among the issue's acceptance criteria. Fixing it is a separate judgement about what a notification key should mean, not part of removing a decode/encode round trip.

test_an_int_and_an_equal_float_ts_deduplicate pins the behaviour that made withdrawing the tag correct, using 1767225600 against 1767225600.0, asserting imported: 0 and the destination unchanged. Re-adding the tag reddens it with AssertionError: an int and an equal float ts stopped deduplicating. The round-3 mutation still reddens too, so both directions of the predicate are pinned.

Same head: 86 / 40 / 51 across test_snapshot.py, test_jsonl_util.py, test_portability.py; isort, black at the pinned --target-version py310, flake8 and mypy clean. The description now records the withdrawal under Key type and in section 5.

[chenmingwei23]

Confirmed and fixed in 21806b448. Reproduced on real bytes before changing anything, because the mechanism you described has a precondition worth pinning down.

The live file holds a crash-truncated row and the source holds a record with a bare carriage return:

live in : b'{"ts":"2026-01-01T00:00:00Z","msg":"a\n'
src  in : b'{"ts":"2026-01-01T00:00:00Z","msg":"a\rb"}\n'

The source splits into two pieces at the carriage return, because this reader's boundaries are the universal-newline set. The FIRST piece strips to exactly the truncated live row, so it was skipped as a duplicate; the second was appended. Result:

live out: b'{"ts":"2026-01-01T00:00:00Z","msg":"a\nb"}\n'

The source's carriage return is gone, the file gained a line that parses as neither, and Notifications imported: 1 was printed. Your finding, verbatim.

The root cause is the general one rather than the carriage return: an unparseable row was keyed on its stripped CONTENT, and content is not identity. Two different records whose stripped bytes coincide collapse, and the loser is deleted. So the fix is your prescription generalised -- a record with no usable ts now yields no key at all and is never deduplicated. Identity is required before anything may be skipped, which is the same posture as abort-not-skip: a skipped record is a permanently deleted one.

Declared cost, stated rather than discovered: re-running a merge re-appends a ts-less row, where the predecessor deduplicated it on stripped bytes. Duplicating a row is recoverable and deleting one is not. test_a_record_with_no_usable_ts_is_never_skipped runs the merge twice and asserts the re-append, so the growth is pinned behaviour rather than a surprise.

One mutation in this round did NOT redden, and diagnosing it removed dead code instead of leaving a false positive in the harness. Writing the skip test as key is not None and key in existing is unobservable: both existing.add sites already refuse None, so None can never be in the set and the conjunct is dead. Deleted rather than kept as untested defensive code. The two add-guards ARE load-bearing, and each now has its own mutation and test -- test_two_distinct_records_with_no_ts_both_land exists because two rows are the smallest input that observes the source-side guard, since with one row there is no second row to be wrongly skipped.

Same head: 89 / 40 / 51 across test_snapshot.py, test_jsonl_util.py, test_portability.py; isort, black at the pinned --target-version py310, flake8, mypy and the lockdown gate clean. Twenty mutations total, the file verified restored byte-for-byte after each.

[chenmingwei23]

Both items dispositioned. Verified each against kirocrew/main rather than against this branch, so neither reading depends on my own diff.

The _merge_crons sibling: confirmed and filed as #8217. Your reading holds exactly. Three refusal paths -- snapshot.py:2350, :2355, and the _usable_cron_shape early return at :2358 -- and portability.py:546 appends crons (merged) unconditionally afterwards. apply_import_zip has no try around the call, so the handler SEL-logs outcome="ok" and answers {"ok": True}. None of those three sees stdout. One detail worth adding to your count: the _usable_cron_shape path does not print at all, so it is quieter than the two you cited -- even a CLI operator sees only the component's own success line.

Filed rather than folded in, for the reason you already accepted: it is pre-existing behaviour in a different component, and the honest fix needs a judgement this PR should not make. Whether a refused cron merge should abort the WHOLE import the way notifications now does is a real design call, because test_merge_still_imports_other_components_after_a_crons_refusal encodes the opposite expectation for multi-component restores. The issue records both candidate shapes and names that test as the thing that decides between them.

The "ts" tuple element: you are right that it discriminates nothing, and I am NOT taking it in this revision. Since the only other return became None, the constant is vestigial -- it dates from when an unparseable row returned ("raw", stripped) and the two kinds had to be told apart. (kind, ts) is the key.

Declining for now is a sequencing judgement, not a disagreement. This head is currently zero-red with the GPT lane still in flight on it, and a push re-rolls five edit-triggered workflows including that lane, which is the one non-deterministic gate here. Trading a settled board for the removal of one constant is a bad exchange while a verdict is pending. If GPT returns a legitimate finding on this head, the subtraction goes in that push, where it costs nothing. If GPT comes back clean, I will leave it and say so, because at that point the only thing a push buys is risk.

Recorded here rather than in the description so this comment re-fires nothing.

[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 #7651. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8184: KEEP. Merged prerequisite, not coverage. The reader and exception types 8184 needs are on main; the notification merge itself is untouched there. Files: src/kiro_crew/jsonl_util.py, test/test_jsonl_util.py.
• This PR is OVERLAPPING with PR #8315. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8184: KEEP. Complementary halves of one reported defect class in the same file, by the same author, with disjoint functions. Neither subsumes the other; only the second to land needs a positional rebase of test/test_snapshot.py. Files: src/kiro_crew/snapshot.py, test/test_snapshot.py.

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

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision 576a636414a7d84b00f4c32bb6a64db6335164b7 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[NicholasRBowers]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause — snapshot notification merge copied records through a locale decode instead of byte-exact, and the dedupe key stripped/collided on non-byte-equal records; now validates UTF-8 and dedupes on kind-tagged keys with raw-bytes fallback.