Repository navigation
Conversation
|
Thanks for the careful work here. The contract discipline is visible throughout: fail-closed provenance, no callbacks inside the write transaction, an additive surface, and docs that do not overclaim. I reviewed head 4808cab against the diff and ran the three new test files plus the delete-chain regression suite. Test note, in the spirit of not softening: on a Python 3.11 environment, 23 of your 25 new tests pass. The two exceptions -- test_receipt_preserves_only_routing_identifiers and the cross-process handoff test -- fail because agent/turn_scripted_prelude.py:24 uses the PEP 696 two-argument typing.Generator[Y, S] form, which only parses on Python 3.13 or newer (the file is untouched by this PR; clean main reproduces it identically). With the semantically identical three-argument annotation, both pass, and the focused set is 34/34 green. Worth flagging separately to maintainers that requires-python (>=3.11) is out of sync with that file. Nothing in this PR causes it. Findings, all small:
On your open question: keep the receipt store; do not add a constrained transactional writer callback. A callback inside the deletion transaction either re-enters SQLite under the write lock or performs external work that a rollback can orphan -- which is precisely the coupling the observer was kept away from. The clean split is receipt = durable, fail-closed truth (this PR) and post-commit observer = best-effort wake-up (#124988), exactly as your docs frame it. If anything, keep constraining the receipt surface rather than growing a veto path. Happy to iterate, and glad to help pressure-test the consumer cursor/reclaim protocol once maintainers weigh in. |
|
@Finn763, thanks for the concrete review and for exercising the Python 3.11 path. I am addressing the receipt-reader retry path with regression coverage, the unreconciled-schema error contract, sidebar registration, and an explicit list of uninstrumented CLI deletion surfaces in an isolated checkout. I will retain the receipt-store design without transaction callbacks and keep this draft pending verification and maintainer decisions; the delegate-traversal optimization will stay separate unless needed for correctness. I also noticed #124988 was closed without merging. Could you clarify the reason and whether a replacement or follow-up observer PR is planned? I will not assume the observer is available in main or fold your implementation into this PR. This draft remains a receipt/replay surface, with any wake-up integration separately verified. |
Independently reviewed; focused regression tests and official checks pass. Full suite and observer integration remain unverified.
|
@Finn763, the review follow-up is pushed as b8e04c9:
Verification of the exact committed source on macOS/Python 3.14: nine focused files 97 passed, RPC deletion neighbors 8 passed, official 11 checks passed. Six shared regression files on the fixed separate recorded base 68 passed. I separately reran the receipt file (26 passed), verified source/patch hashes, and obtained independent static patch review with no security/logic blockers. Initial inherited-HOME harness failures were retained and rerun with disposable HOME; they are not product fixes or full-suite successes. Full state-suite/website/native integration remain unverified. Separately reproduced the Python 3.11 The receipt-store/no-transaction-callback design remains. Keeping this Draft pending maintainer decisions on retention/all-consumer coordination and workflow approval; a re-review of this narrow follow-up would be welcome. My earlier question about #124988's closure/replacement remains open; no observer integration is assumed available. |
|
Thanks for the turnaround -- the |
Preserve upstream onboarding delete_chat ordering and user_rpc deletion receipts. Retain active-write/rollback guards; add explicit subprocess check=False for current lint rules. Independent static review passed; scoped 140 tests and 12 checks pass. No whole-suite or native integration claim.
|
Conflict refresh pushed as b72c6d5, merging frozen upstream Retained upstream onboarding delete_chat wrapping/order and caller-authored user_rpc provenance, active-write protection and transactional receipts. No arbitrary transaction callback added. Two subprocess calls explicitly use check=False for current lint, preserving return-code assertions. Final 14-file regression: 140 passed; official scripts/check: 12 checks OK, health 0/0. Real onboarding RPC probe verified receipt commit before notification and no extra receipt on re-delete. Full state suite/website/native/plugin integration and retention coordination remain unverified; Draft retained. Independent static review found no security/logic blockers, and the committed tree equals the tested/reviewed candidate. Separate merge-tree check against latest main |
Retain upstream canonical mutation/revision/lineage retirement and onboarding behavior. Bridge server-authored REST provenance through private authority arguments; capture final eligible lineage atomically. Independent static review passed; canonical regression 243 passed and 1 Darwin platform skip, 12 checks pass. No external cleanup features or full-suite/native claims.
|
Recurrent conflicts resolved and pushed as 27d9b69, merging upstream Kept upstream canonical REST mutation/ownership/revision/generation/idle/retirement/onboarding behavior rather than restoring the legacy direct delete writer. A minimal private handler-authored provenance bridge retains REST receipts; final eligible reset/compression/delegate identities and deletion/mutation receipts commit or roll back together. No new plugin, external cleanup executor or callback. Final canonical regression 243 passed, 1 Linux-only skip on Darwin across 27 files; all 12 blocking checks OK. Published tree equals tested/reviewed tree. Independent static patch/helper review found no security/logic blockers. Nonblocking transport-spoof and combined lineage integration coverage suggestions remain explicit in the body. Keeping Draft and existing maintainer design/retention/CI gates. No upstream merge, production changes, or full-suite/native E2E claim. |
What does this PR do?
Independent generic follow-up to #124988, based on recorded
mainsnapshot at5f045f842a60184748dda30acb9fecbd961cc18b. It adds caller-authored explicit-user deletion provenance and transactionally prepared minimal routing identities, exposed through publicSessionDBreceipt APIs for a plugin-owned durable backend-to-gateway handoff.Draft / design decision requested: is a small generic transactional receipt store the preferred extension, and what retention/consumer-coordination policy should accompany it? The safe core piece is implemented and exercised; no synchronous plugin callback is run inside a live SQLite deletion transaction. If maintainers require a callback beyond the prepared-identity/read surface, its constrained writer/rollback semantics need agreement first. This draft is not a claim that the complete external cleanup/migration acceptance criteria are approved.
Related Issue
on_session_deleteplugin hook (fire on every session-deletion path) #124511, not a competing observer implementation.Type of Change
Changes Made
session.deleteRPC handlers setdeletion_origin; no new wire parameter lets a client author its own provenance.Concrete consumer: an opt-in standalone plugin pairs an external messaging thread with stored conversation history and its own proven-owned files. It needs explicit user intent and routing identifiers even when Desktop commits in one process and the gateway consumes in another. That external policy is intentionally not added to core.
How to Test
Prepared an isolated PM Python 3.14 dev/test environment (
python -m pm.build_env --source . --out .venv --group dev --group test), with a disposable development home and no copied credentials. Tested on macOS arm64.The cross-process test executes the real RPC handler against an actual temporary
SessionDB, exits the backend abruptly after commit, then starts independent consumer processes which persist/restart a plugin-owned SQLite pending ledger using only public receipt reads. No external service is contacted.Red-to-green observations: missing state keyword/API, missing REST provenance (zero receipts), missing RPC provenance in the cross-process handoff (zero receipts), and missing reclamation API. Contract tests additionally cover rollback, no-op/fences, active turn and compression guards, recursive delegate/compression IDs, profile isolation, real prune/move/empty paths, schema reconciliation, replay bounds and monotonic cursor reclamation.
Limitations / not green claims
tests/hermes_state/run reported failures and exceeded the execution timeout; not a full-suite pass. Two representative failing files were re-run against untouched base:test_guest_durability_barriers.py(1 failure) andtest_cross_vm_fs_wal_refusal.py(3 failures), reproducing the same failures. Other broad-run failures are not all baseline-classified.hermes sessions delete, interactive session browser deletion, and/exit --deleteremain uninstrumented; intent must not be inferred from row absence.Checklist
Review follow-up — b8e04c9
_read_all/_read_retrying_ioerr. Regression coverage injects execute/fetchall transient IOERR and verifies replay on a real read-only connection; this is the primary read-only connection, not writable-reader-pool coverage.sqlite3.OperationalError: no such table: session_deletion_receiptsfor list/get; contract tests/documentation prevent interpreting schema-not-ready as empty replay.website/sidebars.ts; named the three unsupported CLI deletion surfaces. Delegate-traversal optimization is deferred as a nonblocking performance nit. No transaction callbacks added.typingimport/Preludealias reproducesToo few arguments for typing.Generator; actual 2, expected 3on both this PR's source and recorded base. This is not a full Python 3.11 module import or pytest run; no unrelated compatibility fix is included.Conflict refresh — b72c6d5
Earlier test sections are historical pre-refresh evidence. This commit merges frozen upstream
e06df9c2eae6901f4c88f464e0ae6e753c0428b8; published tree exactly matches the tested/reviewed candidate.Retained upstream onboarding delete_chat wrapping/order and caller-authored user_rpc provenance, active-write protection and transactional receipts. No arbitrary transaction callback added. Two subprocess calls explicitly use check=False for current lint, preserving return-code assertions. Final 14-file regression: 140 passed; official scripts/check: 12 checks OK, health 0/0. Real onboarding RPC probe verified receipt commit before notification and no extra receipt on re-delete. Full state suite/website/native/plugin integration and retention coordination remain unverified; Draft retained.
Independent static review found no security/logic blockers; this does not imply the reviewer executed suites. PR-delta whitespace is clean; unrelated upstream whitespace findings were retained. Latest main
d97007c32d6340f7ba79ac1dfb5773cc00846448was separately checked with merge-tree (exit 0); its newer delta is release-tool scripts/tests. This is a conflict check, not runtime validation of a synthetic combined tree. Exact-head GitHub readback reportedmergeable=true,mergeable_state=blocked; content conflicts are cleared, but review/design/workflow gates remain. No upstream PR merge, production code/database change, app edit or restart performed.Recurrent conflict refresh — 27d9b69
Merged frozen upstream
7bda2a3368024f7ae1c70cef16583e7c2c891674. The REST conflict keeps upstream canonical authenticated/owner-fenced revision/generation/idle mutation instead of restoring the old direct writer. A minimal private Python_deletion_originbridge preserves handler-authored explicit REST provenance through authority/runtime helpers; client payloads do not control provenance. State deletion captures final eligible local-reset/compression/delegate scope before retirement/deletion in the same transaction. Exact retry returns before preparation; mutation-receipt failure rolls preparation/deletion back together. No external cleanup executor, new plugin or transaction callback added.Actual canonical regression: 27 files, 243 passed, 0 failed, 1 skipped (Linux-only lineage case skipped on Darwin). Blocking checks: 12 OK, health 0 blocking/0 advisory with explicit frozen upstream base. Exact published tree
f07ddcd52750cedfccd0de9fda4a995c528762ffmatches the tested candidate; tracked-source hashes were unchanged across final execution. Real HTTP regressions exercise live-owner/standalone receipts, stale/exact retry, profile isolation and rollback; state coverage includes reset lineage and queued-work refusal.Independent static review of the full upstream-relative patch and surrounding authority/transaction helpers found no blocking security/logic defects. Nonblocking coverage suggestions remain: HTTP/WS-level provenance-spoof transport tests and a combined reset/compressed-delegate/surviving-branch receipt-vs-deleted-ID test. These are not claimed covered; reviewer did not execute suites. No full-suite/native integration or maintainer approval claim. Draft and retention/design/remote-CI gates remain.