Skip to content

feat: add lossless-ignored sidecar table and store API (WS1.5 PR-1) - #162

Closed
100yenadmin wants to merge 6 commits into
mainfrom
upstream/pr-362
Closed

feat: add lossless-ignored sidecar table and store API (WS1.5 PR-1)#162
100yenadmin wants to merge 6 commits into
mainfrom
upstream/pr-362

Conversation

@100yenadmin

@100yenadmin 100yenadmin commented Aug 16, 2026

Copy link
Copy Markdown
Member

Important

This executable LCM-X PR was recreated by the migration operator from the exact upstream commit head. GitHub did not transfer the original PR actor, dates, review objects, or approval state.

Source and attribution

@coderabbitai ignore

Original commit authorship and history remain in the commits. Historical discussion and review text are imported below as attributed ordinary comments; they are not new approvals or change requests.

Original upstream PR description

Summary

This remains the behavior-neutral storage substrate for preserving messages dropped by ignore_message_patterns without exposing them to replay, FTS, token counts, or cursor reconciliation.

  • Add the ignored_messages child table and idx_ignored_session through the idempotent versioned-migration path.
  • Add sidecar append/read/count APIs with the same ingest protection as normal message storage.
  • Keep normal and ignored rows atomic across session reassignment, deletion, and /lcm doctor clean apply.
  • Include ignored-only sessions in cleanup discovery and preserve backup-first cleanup behavior.
  • Reject mismatched ignored-message metadata lengths before opening a transaction, avoiding zip(...) truncation and partial writes.
  • Make table/index bootstrap safe under concurrent opens.

The reviewed head is e83a818a135a298682dfe91e1bb46e6a0c8aa3e3.

Why

An ignored column on the hot messages table would break the project’s stored-row/replay-row assumptions and allow external-content FTS rebuilds to index ignored content. A separate child table keeps ignored content structurally outside replay, FTS, counts, and reconciliation while retaining it for audit and explicit inspection.

The repair also closes the lifecycle gaps identified during review: ignored-only sessions are discoverable by cleanup, cleanup deletes sidecar rows atomically, reassignment/deletion roll back both normal and sidecar mutations on failure, and optional metadata cannot silently truncate a batch.

Validation

Exact reviewed head:

PYTHONPATH=/tmp/t_a1dc5f84-generated python /tmp/hermes-verify-a1dc5f84.py
# 3 passed

PYTHONPATH=/tmp/t_a1dc5f84-generated python -m pytest tests/test_ignored_sidecar.py tests/test_lcm_command.py -q
# 92 passed

python -m pytest tests/ -q
# 1627 passed, 12 xfailed

ruff check db_bootstrap.py store.py command.py tests/test_ignored_sidecar.py tests/test_lcm_command.py
# All checks passed

python -m py_compile db_bootstrap.py store.py command.py tests/test_ignored_sidecar.py tests/test_lcm_command.py
# passed

git diff --check upstream/main..HEAD
# passed

The effective added-line security scan over upstream/main..HEAD reported 0 findings. Independent same-card review ACCEPT reproduced the 92 focused tests and the 3 critical concurrency/rollback regressions on the exact head.

Notes

  • ensure_ignored_messages_table remains idempotent and does not advance SCHEMA_VERSION; older builds ignore the extra table, preserving downgrade behavior.
  • Column layout mirrors messages plus matched_pattern; ignored content remains audit-on-demand rather than active context.
  • No ingest path is redirected in this PR. Capture wiring and status/inspect surfacing remain in the follow-up PR.
  • The branch update was a normal fast-forward from 5033da015819cacbd3b8b87248bfdca9b8221edb; no force-push or history rewrite was used.

Refs

  • Prior review findings addressed: sidecar cleanup deletion, ignored-only cleanup candidates, and ignored-batch metadata length validation.
  • lossless-claw sidecar model: message_parts.is_ignored.
  • Rejected alternative: an in-table messages.ignored flag, which would reintroduce FTS rebuild and reconciliation coupling.

ai-ag2026 and others added 6 commits July 12, 2026 02:23
@ai-ag2026 @claude Manfred
feat: add lossless-ignored sidecar table and store API (WS1.5 PR-1)
f5c9f1d 
Groundwork for storing ignore_message_patterns drops losslessly instead
of discarding them. Adds an additive ignored_messages child table plus
the store read/write API, with no ingest behavior change yet: no drop
path is redirected here, so replay, FTS, token counts, and cursor
reconciliation are untouched.

The sidecar is deliberately separate from the messages table. Keeping
ignored content out of the parent means the ~10 'stored row == replay
row' reconciliation assumptions and the external-content FTS rebuild
never see it, so lossless capture needs no changes to those fragile
subsystems (the reason a messages.ignored column was rejected).

Migration is version-fenceless: ensure_ignored_messages_table runs
unconditionally in run_versioned_migrations and does not bump
SCHEMA_VERSION, so an older build simply ignores the table and keeps
dropping -- downgrades stay safe. append_ignored_batch routes rows
through the same ingest protection as append_batch (redaction /
externalization before persist). Session rebind and delete keep the
sidecar bound to its parent session.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ai-ag2026 Manfred
fix: purge ignored sidecar during doctor cleanup
85c1b48
@ai-ag2026 Manfred
fix: discover ignored-only cleanup sessions
bfba9f2
@ai-ag2026
fix: reject truncated ignored sidecar metadata
5033da0
@ai-ag2026
Merge upstream main into ignored sidecar repair
88a1765
@ai-ag2026
fix: make ignored sidecar lifecycle atomic
e83a818
@100yenadmin

100yenadmin commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Imported upstream pr-conversation

@​codex review current head 9bcbc27. Base PR for the lossless ignored-message sidecar stack; please review the schema/store API boundary and migration/test coverage before the follow-on capture PR is considered.

Imported upstream pr-conversation

@​codex review current head b22be578. Addressed the cleanup P2: doctor clean apply now deletes ignored_messages in the same transaction, with purge and rollback regression coverage. Validation: exact regressions 2 passed; adjacent 85 passed; stacked full suite 1622 passed, 12 xfailed; ruff and hygiene gates passed.

Imported upstream pr-conversation

Re-gate evidence for current head ea5edcf30ca3e9b9e800c0853b2d772db8d4e5b4:

  • fixed the active ignored-only cleanup finding by adding ignored_messages.session_id to scan_session_cleanup_stats() candidate discovery;
  • added an ignored-only preview/apply regression; RED on old head, GREEN after repair;
  • focused cleanup/rollback tests: 4 passed; full suite: 1616 passed, 12 xfailed; static/diff hygiene passed;
  • exact fork and PR refs verified.

@​codex please review this exact head, especially ignored-only discovery and cleanup atomicity.

Imported upstream pr-conversation

@​codex Please review repaired head ea5edcf30ca3e9b9e800c0853b2d772db8d4e5b4.

Repair: cleanup candidate discovery now includes ignored-sidecar-only sessions; regression covers preview discovery and backup-first apply deletion. Transactional rollback coverage remains green.

Fresh local evidence: focused 4 passed; full suite 1616 passed / 12 xfailed; ruff, compileall, py_compile, shell syntax, and git diff --check passed. GitHub Actions: 6/6 check runs completed successfully.

Please focus on the candidate CTE scope and ignored-only cleanup semantics.

Imported upstream pr-conversation

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: ea5edcf30c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@​codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@​codex address that feedback".

Imported upstream pr-conversation

@​codex review\n\nCurrent repaired head: 5033da015819cacbd3b8b87248bfdca9b8221edb. Please review the current head; the branch was updated with an exact force-with-lease and fork/PR readback matches.

Imported upstream pr-conversation

@​codex review

Please review exact repaired head e83a818a135a298682dfe91e1bb46e6a0c8aa3e3.

This fast-forward repair addresses the three current P2 findings: metadata-length validation before transactions, ignored-only cleanup discovery with backup-first deletion, and atomic normal/sidecar reassignment, deletion, and cleanup rollback. Independent local review reproduced 92 focused tests plus 3 critical concurrency/rollback regressions.

Imported upstream pr-conversation

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: e83a818a13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@​codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@​codex address that feedback".

Imported upstream pr-conversation

Moved to draft to reduce the size of the open review queue, as requested.

Nothing is closed and no work is lost: the branch and its history stay intact. This will be reopened once the corresponding workstream base has clear review direction. Queue context: #416.

This PR is the base of the WS1.5 stack — #363 builds on it — so it is one of the first that would be reopened.

Imported upstream pr-review-commented

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9bcbc27c13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@​codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@​codex address that feedback".

Imported upstream pr-review-commented

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b22be57815

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@​codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@​codex address that feedback".

Imported upstream pr-review-commented

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ea5edcf30c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@​codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@​codex address that feedback".

Imported upstream pr-review-comment

P2 Badge Delete sidecar rows during cleanup

When the follow-on capture path starts using this insert, /lcm doctor clean apply will leave these rows behind: I checked _delete_clean_candidates_atomically in command.py, and its transaction deletes only from messages, summary_nodes, and lcm_lifecycle_state (command.py:1291-1297). For any ignored/stateless session with ignored sidecar rows, the normal rows and lifecycle state are removed but ignored_messages still retains the dropped content, so destructive cleanup no longer purges all stored data for that session. Please include ignored_messages in the cleanup apply path before enabling capture.

Useful? React with 👍 / 👎.

Imported upstream pr-review-comment

P2 Badge Include ignored-only sessions in cleanup candidates

When a session has only rows in ignored_messages (which append_ignored_batch now permits, and will be common once ignore_message_patterns drops are redirected), /lcm doctor clean never discovers it because MessageStore.scan_session_cleanup_stats() only unions messages and summary_nodes. In that scenario doctor clean apply never reaches this new ignored_messages delete, so matched junk sessions with only ignored sidecar rows remain indefinitely; include ignored_messages in the cleanup scan/candidate stats as well as in the apply delete.

Useful? React with 👍 / 👎.

Imported upstream pr-review-comment

P2 Badge Validate ignored batch metadata lengths

When callers provide token_estimates or matched_patterns, this zip(...) truncates to the shortest list. For example, passing matched_patterns=[] to mean “no pattern labels” with a non-empty ignored batch inserts zero rows, and a one-short token list silently drops the tail ignored messages, defeating the sidecar’s lossless audit purpose. Pad missing metadata with defaults or reject length mismatches before starting the transaction.

Useful? React with 👍 / 👎.

@100yenadmin 100yenadmin mentioned this pull request Aug 16, 2026
Closed
@100yenadmin 100yenadmin added accepted-defer Concrete accepted work deferred until its recorded revisit trigger active-continuation Active continuation of an attributed upstream item enhancement New feature or request eva-possible Possible Electric Sheep impact requiring path-specific confirmation P3 Limited bug, edge case, non-blocking feature, or moderate maintainability issue upstream-evidence Preserves links and attribution to the upstream report or pull request upstream-pr Imported upstream pull-request evidence labels Aug 16, 2026
This was referenced Aug 19, 2026
Closed
Closed
@100yenadmin

100yenadmin commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Triage: this PR is fully contained in #163.

Established by commit-set inclusion, not inspection — every commit on this branch is an ancestor of #163's head, and #163 carries additional commits on top. So landing #163 lands this work too, and this PR should close as included at that point rather than being reviewed separately.

Not closing it yet, because #163 is not itself ready: it needs a rebase against current main. If #163 is ultimately abandoned, this branch becomes the fallback, so it stays open until that is decided.

One correction to what its checks currently show: the red on this PR is stale, not broken. It failed Analyze (javascript-typescript) / CodeQL on 2026-08-16 with CodeQL could not process any code written in JavaScript/TypeScript, exit code 32 — this is a Python repo, and that job has passed on every PR landed since. It needs a re-run, not repair. Tracked in #241, where I've recommended dropping those contexts from required precisely because they make PRs look broken when they are not.

@100yenadmin 100yenadmin mentioned this pull request Sep 7, 2026
Closed
@100yenadmin

100yenadmin commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

Thank you @ai-ag2026. This draft mirrors upstream hermes-lcm#362, which was closed upstream without being merged. Its preservation record (#99) is closed. It is a behaviour-neutral schema substrate on a base about 1,200 commits behind main, which has since shipped v0.24.0 and v0.24.1. Closing as a stale mirror. Please refile against current main if the ignored-message sidecar is picked up again.

@100yenadmin 100yenadmin mentioned this pull request Sep 25, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

accepted-defer Concrete accepted work deferred until its recorded revisit trigger active-continuation Active continuation of an attributed upstream item enhancement New feature or request eva-possible Possible Electric Sheep impact requiring path-specific confirmation P3 Limited bug, edge case, non-blocking feature, or moderate maintainability issue upstream-evidence Preserves links and attribution to the upstream report or pull request upstream-pr Imported upstream pull-request evidence

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@100yenadmin @ai-ag2026