Skip to content

fix: resolve issues found by post-merge review (PR #40) - #40

Merged
chiruu12 merged 7 commits into
mainfrom
fix/review-findings
Jun 2, 2026
Merged

fix: resolve issues found by post-merge review (PR #40)#40
chiruu12 merged 7 commits into
mainfrom
fix/review-findings

Conversation

@chiruu12

@chiruu12 chiruu12 commented Jun 2, 2026

Copy link
Copy Markdown
Owner

Summary

PR #40 — fixes for the issues found by the post-merge review agents (run after #34/#35/#38/#39 landed on main). Six small commits, full gate green: 1031 tests, ruff/format/mypy/mkdocs clean.

Fixes

Sev Fix Commit
🔴 HIGH Event log shredded valid recordsreplay()/stream() used str.splitlines(), which also splits on \r/\f/NEL/U+2028/U+2029 (legal unescaped in JSON). An event whose text carried one was mis-parsed as corruption (replay raised, stream crashed). Now split on \n only + rstrip("\r"). 903c8c5
🟢 LOW Event-log fsync now also fsyncs the parent directory on file creation (durable link). fsync dir
🟢 LOW Dropped dead HIVE_MAX_TURNS/HIVE_SESSION_TIMEOUT env mappings (no backing ModelConfig fields → silently ignored). config
🟢 LOW Checkpoint snapshots the freshly-saved identity (was stale after update_narrative); the crisis directive is no longer duplicated in the pursuit prompt. daemon
🟢 LOW get_pending_nudges marks only the nudges it read as delivered (by id) — a concurrently-inserted nudge isn't lost. store
🟡 MED CHANGELOG: the 0.5.4 entry now covers mood, chaptered narrative, narrative-in-prompt, and F1 (it only listed durability before) + a Fixed section. changelog

Each fix has a regression test where applicable (Unicode-separator replay, nudge-delivery-once, etc.).

Not changed (deliberate)

  • MoodRegistry.set_model process-global singleton — intentional, consistent with StressorRegistry.
  • v0.5.3/v0.5.4 tags remain unpushed — the release is tagged after this merges.

Closes out the review pass over #34/#35/#38/#39. After this merges, the plan is to tag and release v0.5.4.

chiruu12 added 6 commits June 2, 2026 15:57
str.splitlines() also breaks on \r, \v, \f, NEL (U+0085) and Unicode line
separators U+2028/U+2029 -- all legal *unescaped* inside JSON strings. A
single event whose text (e.g. ASSISTANT_MESSAGE/TOOL_RESULT) carried one
was one physical line with no \n, but splitlines() shredded it, so replay()
raised a spurious 'corruption' error (whole session failed) and stream()
crashed. Split on \n only and rstrip \r for CRLF tolerance.

Test: a record containing NEL round-trips through replay without shredding.
With fsync=True, content was fsync'd but a newly-created session file's
directory entry wasn't durably linked, so a crash could lose a just-created
(content-fsync'd) file. fsync the parent dir once on file creation; guarded
for filesystems that don't support directory fsync.
…ings

These mapped to model.max_turns / model.session_timeout, which ModelConfig
does not define -- pydantic silently ignored them, so setting either env
var was a no-op. Removed the dead rows (the remaining mappings all target
real fields).
- update_narrative reloads/saves its own identity copy, so the in-memory
  identity passed to checkpoint.save was stale (missing the just-appended
  entry/chapter). Reload before snapshotting.
- Skip the derived mood line when in crisis: the suffering fragment already
  emits the crisis directive, so 'overwhelmed' duplicated it in the prompt.
get_pending_nudges did SELECT (delivered=0) then UPDATE delivered=1 for the
whole agent, so a nudge inserted between the two statements (e.g. by another
process) was marked delivered without being returned -- silently lost. Now
it updates only the nudge_ids actually read. Test: a nudge added after a
read is still delivered exactly once on the next read.
… + F1 in 0.5.4

The 0.5.4 entry listed only the durability work, but #34/#35/#38/#39 all
merged into this unreleased version. Add Added entries (mood model,
chaptered narrative, narrative-in-prompt, F1 coverage) and a Fixed section
for the review follow-ups. Mirrored in docs/changelog.md.
Copilot AI review requested due to automatic review settings June 2, 2026 10:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses issues surfaced by post-merge review of recently merged PRs (#34, #35, #38, #39). It includes one HIGH-severity correctness fix (event log no longer shreds records containing Unicode line separators), several low-severity hardening fixes, and a CHANGELOG update covering all 0.5.4 features.

Changes:

  • Replace str.splitlines() with "\n" split + rstrip("\r") in EventLog.replay()/stream() so records containing \r, \f, NEL, U+2028, or U+2029 are not shredded; add directory fsync after creating a new event log file.
  • Tighten correctness: get_pending_nudges marks only the rows it actually read by id; checkpoints reload identity after update_narrative; crisis suppresses the (duplicative) mood line; drop dead HIVE_MAX_TURNS/HIVE_SESSION_TIMEOUT env mappings.
  • Expand CHANGELOG 0.5.4 entry (mood, chaptered narrative, narrative-in-prompt, F1) and add a Fixed section; new regression tests for unicode-separator replay and once-only nudge delivery.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/hive/memory/events.py Split on \n only (not splitlines()), tolerate CRLF, fsync parent dir on file creation.
src/hive/memory/store.py get_pending_nudges marks delivered by id of returned rows only.
src/hive/daemon/loop.py Suppress mood line during crisis; reload identity before checkpoint snapshot.
src/hive/config.py Remove unmapped HIVE_MAX_TURNS/HIVE_SESSION_TIMEOUT env entries.
tests/test_events.py Regression test for records containing U+0085 (NEL).
tests/memory/test_store.py Regression test that a concurrently inserted nudge isn't lost.
docs/changelog.md, CHANGELOG.md Expand 0.5.4 notes with mood, chaptered narrative, narrative-in-prompt, F1, and a Fixed section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps

greptile-apps Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Six targeted bug fixes identified during the post-merge review of PRs #34#39. All changes are surgical and each is accompanied by a regression test where applicable.

  • Event log line splitting (events.py): replay() and stream() now split on \ only instead of str.splitlines(), preventing Unicode line-separator characters (NEL, U+2028, U+2029, etc.) from shredding records that legally carry them in their JSON text. A new test covers the NEL round-trip.
  • Nudge delivery race (store.py): get_pending_nudges now marks only the fetched IDs as delivered via WHERE nudge_id IN (...), closing the window where a concurrently-inserted nudge could be swept up and silently lost; the new concurrent-insert monkey-patch test validates the exact race window.
  • Daemon loop (loop.py): mood line is skipped when suffering.in_crisis (the crisis directive already covers it), and identity is reloaded after update_narrative so the checkpoint snapshot reflects the freshly-written narrative entry.
  • Config cleanup (config.py): dead HIVE_MAX_TURNS/HIVE_SESSION_TIMEOUT env mappings removed; event log now also fsyncs the parent directory on file creation for durable directory linking.

Confidence Score: 5/5

All six changes are well-scoped fixes with clear regression tests; no existing behavior is removed, only incorrect behavior is corrected.

Each fix targets a specific, demonstrable bug (Unicode record shredding, stale checkpoint identity, crisis-directive duplication, dead config keys, missing directory fsync, nudge delivery race) with a narrow code footprint. The nudge fix is correctly implemented — the SELECT and UPDATE share a single connection, the placeholders are fully parameterised, and the race window test exercises the actual injection point. The event log splitting fix is logically sound: split("
") + rstrip("
") correctly handles CRLF and leaves Unicode line separators inside JSON strings intact.

No files require special attention.

Important Files Changed

Filename Overview
src/hive/memory/events.py Replaces str.splitlines() with str.split("\n") + rstrip("\r") in replay() and stream() to avoid shredding records that contain Unicode line separators; adds parent-directory fsync on new-file creation when fsync=True.
src/hive/memory/store.py Fixes get_pending_nudges to mark only the fetched nudge IDs as delivered via WHERE nudge_id IN (...) instead of WHERE delivered = 0, preventing a concurrently-inserted nudge from being silently lost.
src/hive/daemon/loop.py Two small fixes: skips mood line when suffering.in_crisis to avoid duplicating the crisis directive, and reloads identity after update_narrative so the checkpoint snapshot reflects the freshly-persisted narrative.
src/hive/config.py Drops two dead env-var mappings (HIVE_MAX_TURNS, HIVE_SESSION_TIMEOUT) that had no backing ModelConfig fields and were silently ignored.
tests/test_events.py Adds test_replay_handles_unicode_line_separators using U+0085 (NEL) in event data; correctly validates that replay() returns both records intact and that the tricky payload survives round-trip.
tests/memory/test_store.py Two new nudge tests: a sequential test for by-id delivery, and a sophisticated monkey-patch test that injects a concurrent insert between the SELECT and the UPDATE to validate the race window is closed.
CHANGELOG.md Expands the 0.5.4 entry to cover mood, chaptered narrative, narrative-in-prompt, and F1 test coverage; adds a new Fixed section for all six bug fixes in this PR.
docs/changelog.md Mirrors the CHANGELOG.md additions for the docs site copy.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant HiveStore
    participant DB as SQLite (aiosqlite)
    participant OtherProcess

    Note over Caller,OtherProcess: get_pending_nudges — fixed by-id UPDATE

    Caller->>HiveStore: get_pending_nudges("agent-1")
    HiveStore->>DB: "SELECT nudge_id, message WHERE delivered=0"
    DB-->>HiveStore: [(n1, "first"), (n2, "second")]

    Note over OtherProcess: Race window: new nudge inserted
    OtherProcess->>DB: "INSERT nudge (n3, "third", delivered=0)"

    HiveStore->>DB: "UPDATE SET delivered=1 WHERE nudge_id IN (n1,n2)"
    Note over DB: n3 NOT marked delivered (old code would have swept it)
    DB-->>HiveStore: OK
    HiveStore->>DB: COMMIT
    HiveStore-->>Caller: ["first", "second"]

    Caller->>HiveStore: get_pending_nudges("agent-1")
    HiveStore->>DB: "SELECT nudge_id, message WHERE delivered=0"
    DB-->>HiveStore: [(n3, "third")]
    HiveStore->>DB: "UPDATE SET delivered=1 WHERE nudge_id IN (n3)"
    HiveStore-->>Caller: ["third"] — not lost
Loading

Reviews (2): Last reviewed commit: "test(store): exercise the actual nudge S..." | Re-trigger Greptile

Comment thread tests/memory/test_store.py
The prior test only validated sequential delivery -- the old
WHERE delivered=0 sweep passed it too. New test commits a competing nudge
from a separate connection in the window between the SELECT and the UPDATE
(via a sync execute wrapper that preserves aiosqlite's await/async-with
duality). It fails against the old sweep (n2 marked delivered and lost) and
passes with the by-id UPDATE.
@chiruu12
chiruu12 merged commit a1a30cf into main Jun 2, 2026
6 checks passed
@chiruu12
chiruu12 deleted the fix/review-findings branch June 2, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants