Skip to content

fix(sidebar): keep manual forks out of compaction snapshot lineage swaps (#3799) - #3884

Closed
rodboev wants to merge 6 commits into
nesquena:masterfrom
rodboev:pr/compaction-fork-lineage-boundary
Closed

rodboev wants to merge 6 commits into
nesquena:masterfrom
rodboev:pr/compaction-fork-lineage-boundary

Conversation

@rodboev

@rodboev rodboev commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

  • Manual forks are user-visible branches, but the sidebar compaction-grouping path was still walking through them as if they were compression continuations.
  • The direct fix is to respect explicit fork boundaries in the sidebar lineage-root logic, but that must not hide child-session rows that attach under hidden compression segments through enriched lineage metadata.
  • The final change therefore narrows the fork boundary to explicit manual forks while keeping enriched child-session rows independently visible until the later attachment pass.

What Changed

  • api/models.py: keep explicit manual forks as lineage boundaries in the sidebar snapshot-grouping path, enrich lineage metadata before grouping, and treat enriched child_session rows as their own temporary roots so hidden-compression descendants remain visible.
  • tests/test_session_index.py: add focused regressions for the visible fork child case and the child-under-hidden-compression case.

Why It Matters

Forks are user-visible branches, not implementation-detail continuations. They should remain independently discoverable instead of being collapsed away by compaction snapshot heuristics from their parent lineage.

Verification

  • Targeted: pytest tests/test_session_index.py tests/test_session_lineage_collapse.py tests/test_session_lineage_full_transcript.py tests/test_session_lineage_metadata_api.py tests/test_session_lineage_report.py -v --timeout=60
    Result: passed with an isolated HERMES_WEBUI_TEST_STATE_DIR, 84 passed in 4.76s.
  • Full suite command for reviewer use: pytest tests/ -v --timeout=60
    Result: not run locally in this session.

Risks / Follow-ups

This remains intentionally scoped to sidebar lineage-root parity and hidden-child visibility. It does not add a broader compaction-history discoverability UI, which should stay separate from this correctness fix.

Model Used

GPT-5 via Codex CLI

@greptile-apps

greptile-apps Bot commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes the sidebar compaction-grouping path so manual forks are treated as lineage boundaries rather than transparent compression continuations. It also hardens the background index-rebuild thread against late SESSION_DIR switches and eliminates the SESSION_DIR global re-resolution during a full index rebuild.

  • Sidebar lineage root logic (_sidebar_lineage_root_id): adds three early-return guards — explicit _lineage_root_id, relationship_type == 'child_session', and session_source == 'fork' — so forks remain independently discoverable and enriched child-session rows stay visible as their own roots.
  • Ordering fix (all_sessions): _enrich_sidebar_lineage_metadata is now called before _prefer_fuller_snapshots_for_sidebar so enrichment-supplied metadata is available during snapshot grouping on both the index path and the full-scan fallback path.
  • TOCTOU fix (_rebuild_session_index_background): the background thread captures its target (SESSION_DIR, SESSION_INDEX_FILE) at spawn time, validates them inside the lock before writing, and uses _load_session_from_path to load session files from the explicit captured path, preventing a mid-flight session-dir switch from redirecting the write.

Confidence Score: 5/5

Safe to merge; the logic changes are well-scoped and the new background-rebuild guard correctly prevents cross-directory index writes.

The three new branches in _sidebar_lineage_root_id are additive and ordered correctly after enrichment. The explicit-path threading fix eliminates the TOCTOU window identified in a previous review cycle. No pre-existing interface is broken.

tests/test_session_index.py — the integration assertion for the fork test is missing a total-row count check.

Important Files Changed

Filename Overview
api/models.py Moves _enrich_sidebar_lineage_metadata before _prefer_fuller_snapshots_for_sidebar, adds fork/child_session early-returns to _sidebar_lineage_root_id, adds _load_session_from_path to avoid SESSION_DIR re-resolution in full rebuild, and hardens background rebuild thread with captured target tracking to prevent cross-dir writes.
tests/test_session_index.py Adds two new tests: fork visibility regression and late session-dir-switch write guard. The fork test mixes an integration call with direct unit assertions on _sidebar_lineage_root_id; the integration half is missing a len(rows) assertion.

Sequence Diagram

sequenceDiagram
    participant C as all_sessions()
    participant E as _enrich_sidebar_lineage_metadata
    participant P as _prefer_fuller_snapshots_for_sidebar
    participant L as _sidebar_lineage_root_id

    C->>E: result (all non-empty sessions)
    Note over E: sets _lineage_root_id,<br/>relationship_type on entries
    E-->>C: (mutates in-place)
    C->>P: enriched result
    P->>L: per-session root lookup
    L-->>P: explicit _lineage_root_id OR child_session own sid OR fork own sid OR walk parent chain
    P-->>C: snapshot-grouped result
    C->>C: filter hidden, strip flags, backfill profile
    C-->>C: return visible sessions
Loading

Reviews (4): Last reviewed commit: "Requeue the flaky Python 3.13 shard afte..." | Re-trigger Greptile

Comment thread api/models.py Outdated
Comment thread tests/test_session_index.py
nesquena-hermes added a commit that referenced this pull request Jun 9, 2026
…3884) (#3893)

* Release v0.51.344 — Release LH (sidebar fork-lineage grouping #3799/#3884)

Absorbs #3884 (@rodboev): manual forks are kept as sidebar lineage boundaries
so a forked session isn't collapsed under a compression-continuation root,
while enriched child-session rows stay independently visible until the later
attachment pass. Also addresses the greptile TOCTOU flag: the background
index-rebuild thread now pins + re-checks its (SESSION_DIR, SESSION_INDEX_FILE)
target under _SESSION_INDEX_REBUILD_LOCK before writing.

Rebased onto fresh master, content byte-identical to PR head, full-suite +
Codex + Opus gated.

Co-authored-by: rodboev <rodboev@users.noreply.github.com>

* fix(models): propagate target kwargs in index-rebuild fallback (Opus SHOULD-FIX)

Opus advisor stage-344: the _write_session_index fast-path fallback recursed
with _write_session_index(updates=None) and no kwargs, falling back to the
global SESSION_DIR. Safe today (the only kwargs-caller passes updates=None and
never reaches the fast path) but the invariant was implicit. Propagate the
resolved session_dir/session_index_file so a target-scoped rebuild falls back
to that same target.

---------

Co-authored-by: nesquena-hermes <[email protected]>
Co-authored-by: rodboev <rodboev@users.noreply.github.com>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Absorbed and shipped in v0.51.344 (Release LH, deployed live) via the release #3893 — your commit was applied with attribution (rebased onto fresh master, content byte-identical to your PR head, full-suite + Codex + Opus gated). Closes #3799. I also folded in the greptile TOCTOU concern (your threading refactor already addressed it) and applied one Opus-suggested defensive follow-up (propagating the rebuild target through the fast-path fallback); filed #3894 for a remaining low-severity thread-ownership cleanup. Thanks @rodboev! 🙏

nesquena-hermes pushed a commit that referenced this pull request Jun 9, 2026
…obs #3809)

Absorbs contributor PR #3809 (@b3nw), rebased onto fresh master (was ~20 behind,
panels.js conflict resolved by merging the new !isNoAgent skill-tags guard with
the model-select call).

Adds a Model Override dropdown to the Tasks scheduled-jobs create/edit form,
populated from /api/models grouped by provider, persisting model+provider,
clearable to default, disabled in no-agent mode. Surfaces hermes-agent's existing
per-job model override (CLI parity).

greptile P1s (override cleared on fast-save / on API failure) verified
ALREADY-FIXED in PR head; also applied an Opus UX hardening (keep the model
select disabled on a failed /api/models load so the user can't think they
cleared the override). UX approved by Nathan via screenshots.

Pre-merge fixes:
- i18n: the PR added the 3 cron_model_* keys to all locales but left 10 of them
  as 'TODO: translate' English stubs (only es was done), tripping
  test_zh_hant_locale. Provided real translations for de/zh/zh-Hant/ru/ja/fr/pl/
  it/pt/tr.
- test isolation: #3809's new test file shifts pytest-shard composition so
  test_issue2863's background-rebuild test ran after a test that leaves the
  #3884 _SESSION_INDEX_REBUILD_THREAD globals populated, suppressing the fresh
  thread it asserts on. Made that test hermetic (joins+clears the rebuild-thread
  globals up front) so it passes regardless of shard run order.

Co-authored-by: b3nw <b3nw@users.noreply.github.com>
nesquena-hermes added a commit that referenced this pull request Jun 9, 2026
…obs #3809) (#3896)

Absorbs contributor PR #3809 (@b3nw), rebased onto fresh master (was ~20 behind,
panels.js conflict resolved by merging the new !isNoAgent skill-tags guard with
the model-select call).

Adds a Model Override dropdown to the Tasks scheduled-jobs create/edit form,
populated from /api/models grouped by provider, persisting model+provider,
clearable to default, disabled in no-agent mode. Surfaces hermes-agent's existing
per-job model override (CLI parity).

greptile P1s (override cleared on fast-save / on API failure) verified
ALREADY-FIXED in PR head; also applied an Opus UX hardening (keep the model
select disabled on a failed /api/models load so the user can't think they
cleared the override). UX approved by Nathan via screenshots.

Pre-merge fixes:
- i18n: the PR added the 3 cron_model_* keys to all locales but left 10 of them
  as 'TODO: translate' English stubs (only es was done), tripping
  test_zh_hant_locale. Provided real translations for de/zh/zh-Hant/ru/ja/fr/pl/
  it/pt/tr.
- test isolation: #3809's new test file shifts pytest-shard composition so
  test_issue2863's background-rebuild test ran after a test that leaves the
  #3884 _SESSION_INDEX_REBUILD_THREAD globals populated, suppressing the fresh
  thread it asserts on. Made that test hermetic (joins+clears the rebuild-thread
  globals up front) so it passes regardless of shard run order.

Co-authored-by: nesquena-hermes <[email protected]>
Co-authored-by: b3nw <b3nw@users.noreply.github.com>
merodahero pushed a commit to merodahero/hermes-webui that referenced this pull request Jun 13, 2026
…a#3799/nesquena#3884) (nesquena#3893)

* Release v0.51.344 — Release LH (sidebar fork-lineage grouping nesquena#3799/nesquena#3884)

Absorbs nesquena#3884 (@rodboev): manual forks are kept as sidebar lineage boundaries
so a forked session isn't collapsed under a compression-continuation root,
while enriched child-session rows stay independently visible until the later
attachment pass. Also addresses the greptile TOCTOU flag: the background
index-rebuild thread now pins + re-checks its (SESSION_DIR, SESSION_INDEX_FILE)
target under _SESSION_INDEX_REBUILD_LOCK before writing.

Rebased onto fresh master, content byte-identical to PR head, full-suite +
Codex + Opus gated.

Co-authored-by: rodboev <rodboev@users.noreply.github.com>

* fix(models): propagate target kwargs in index-rebuild fallback (Opus SHOULD-FIX)

Opus advisor stage-344: the _write_session_index fast-path fallback recursed
with _write_session_index(updates=None) and no kwargs, falling back to the
global SESSION_DIR. Safe today (the only kwargs-caller passes updates=None and
never reaches the fast path) but the invariant was implicit. Propagate the
resolved session_dir/session_index_file so a target-scoped rebuild falls back
to that same target.

---------

Co-authored-by: nesquena-hermes <[email protected]>
Co-authored-by: rodboev <rodboev@users.noreply.github.com>
merodahero pushed a commit to merodahero/hermes-webui that referenced this pull request Jun 13, 2026
…obs nesquena#3809) (nesquena#3896)

Absorbs contributor PR nesquena#3809 (@b3nw), rebased onto fresh master (was ~20 behind,
panels.js conflict resolved by merging the new !isNoAgent skill-tags guard with
the model-select call).

Adds a Model Override dropdown to the Tasks scheduled-jobs create/edit form,
populated from /api/models grouped by provider, persisting model+provider,
clearable to default, disabled in no-agent mode. Surfaces hermes-agent's existing
per-job model override (CLI parity).

greptile P1s (override cleared on fast-save / on API failure) verified
ALREADY-FIXED in PR head; also applied an Opus UX hardening (keep the model
select disabled on a failed /api/models load so the user can't think they
cleared the override). UX approved by Nathan via screenshots.

Pre-merge fixes:
- i18n: the PR added the 3 cron_model_* keys to all locales but left 10 of them
  as 'TODO: translate' English stubs (only es was done), tripping
  test_zh_hant_locale. Provided real translations for de/zh/zh-Hant/ru/ja/fr/pl/
  it/pt/tr.
- test isolation: nesquena#3809's new test file shifts pytest-shard composition so
  test_issue2863's background-rebuild test ran after a test that leaves the
  nesquena#3884 _SESSION_INDEX_REBUILD_THREAD globals populated, suppressing the fresh
  thread it asserts on. Made that test hermetic (joins+clears the rebuild-thread
  globals up front) so it passes regardless of shard run order.

Co-authored-by: nesquena-hermes <[email protected]>
Co-authored-by: b3nw <b3nw@users.noreply.github.com>
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
…a#3799/nesquena#3884) (nesquena#3893)

* Release v0.51.344 — Release LH (sidebar fork-lineage grouping nesquena#3799/nesquena#3884)

Absorbs nesquena#3884 (@rodboev): manual forks are kept as sidebar lineage boundaries
so a forked session isn't collapsed under a compression-continuation root,
while enriched child-session rows stay independently visible until the later
attachment pass. Also addresses the greptile TOCTOU flag: the background
index-rebuild thread now pins + re-checks its (SESSION_DIR, SESSION_INDEX_FILE)
target under _SESSION_INDEX_REBUILD_LOCK before writing.

Rebased onto fresh master, content byte-identical to PR head, full-suite +
Codex + Opus gated.

Co-authored-by: rodboev <rodboev@users.noreply.github.com>

* fix(models): propagate target kwargs in index-rebuild fallback (Opus SHOULD-FIX)

Opus advisor stage-344: the _write_session_index fast-path fallback recursed
with _write_session_index(updates=None) and no kwargs, falling back to the
global SESSION_DIR. Safe today (the only kwargs-caller passes updates=None and
never reaches the fast path) but the invariant was implicit. Propagate the
resolved session_dir/session_index_file so a target-scoped rebuild falls back
to that same target.

---------

Co-authored-by: nesquena-hermes <[email protected]>
Co-authored-by: rodboev <rodboev@users.noreply.github.com>
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
…obs nesquena#3809) (nesquena#3896)

Absorbs contributor PR nesquena#3809 (@b3nw), rebased onto fresh master (was ~20 behind,
panels.js conflict resolved by merging the new !isNoAgent skill-tags guard with
the model-select call).

Adds a Model Override dropdown to the Tasks scheduled-jobs create/edit form,
populated from /api/models grouped by provider, persisting model+provider,
clearable to default, disabled in no-agent mode. Surfaces hermes-agent's existing
per-job model override (CLI parity).

greptile P1s (override cleared on fast-save / on API failure) verified
ALREADY-FIXED in PR head; also applied an Opus UX hardening (keep the model
select disabled on a failed /api/models load so the user can't think they
cleared the override). UX approved by Nathan via screenshots.

Pre-merge fixes:
- i18n: the PR added the 3 cron_model_* keys to all locales but left 10 of them
  as 'TODO: translate' English stubs (only es was done), tripping
  test_zh_hant_locale. Provided real translations for de/zh/zh-Hant/ru/ja/fr/pl/
  it/pt/tr.
- test isolation: nesquena#3809's new test file shifts pytest-shard composition so
  test_issue2863's background-rebuild test ran after a test that leaves the
  nesquena#3884 _SESSION_INDEX_REBUILD_THREAD globals populated, suppressing the fresh
  thread it asserts on. Made that test hermetic (joins+clears the rebuild-thread
  globals up front) so it passes regardless of shard run order.

Co-authored-by: nesquena-hermes <[email protected]>
Co-authored-by: b3nw <b3nw@users.noreply.github.com>
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