Repository navigation
fix(slack): integrate durable Ring 2 intake - #110466
m0ntydad0n wants to merge 1 commit into
Conversation
|
CI is currently waiting for maintainer approval to run workflows for this fork PR. The branch is rebased onto current main and the reviewed head is |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 160a46aca91c4592ec2cf06c9a356e6fe05c217a against base/current main ee4452991d17534aa561f31ee55596d082aa94e7. The overall direction is strong: the semantic-message ownership boundary, same-receipt waiter behavior, accepted-only-after-dispatch rule, metadata-only HMAC correlation, fail-closed HTTP intake, and opened-object/WAL verification are all materially better than a best-effort in-memory dedup layer. I also checked the new ledger end to end, the Slack adapter integration, current multiplex scoping contract, exact-head CI state, existing review state, commit attribution, and adjacent Slack/dedup work.
I found three blockers on this exact head:
-
The ledger worker drops the routed profile context before resolving its storage path.
_db_path()and_correlation_key()intentionally callget_hermes_home()at operation time, but_run_ledger_work()submits a bare callable to aThreadPoolExecutor. Hermes's profile home is aContextVar; a bare executor worker starts with an empty context. In a multiplex gateway, a secondary Slack adapter therefore enters the ledger worker without its_profile_runtime_scope, and the worker can resolve the process/defaultHERMES_HOMEinstead of the source profile. That collapses the exact owner proof at the persistence boundary: secondary-profile receipts and the correlation key can land in the default profile'sruntime/slack-intake/. Current main already has the canonical solution intools.thread_context.propagate_context_to_thread, and the multiplexing contract explicitly requirescopy_context()across executor boundaries. Please capture the caller context before submission and add a real two-profile regression: process/default home A, context-local routed home B, invoke the async safe entry point, then prove only B's ledger/key changed and A remained untouched. Please audit the process-global fallback/degraded-health buffers at the same boundary so one profile's persistence outage cannot silently become another profile's diagnostic state. -
The durable path is POSIX-only but the PR does not define that as a supported-platform boundary. Every transaction enters
_serialized_connection_lifecycle(), which hard-fails whenfcntl.flockis unavailable;_watch_opened_object()likewise only has kqueue or Linux/proc+inotify implementations. On native Windows this means the claimed durable Ring 2 path is permanently unavailable: Socket Mode degrades to volatile fallback and HTTP intake fails closed. That is not a transient local-test limitation; it is the production control flow. Please either implement an equivalent Windows-safe serialization/object-identity path with adversarial tests, or make an explicit repository-supported platform decision and scope the feature/docs/tests accordingly. The current body presents this as general Slack intake durability and does not disclose a native-Windows exclusion. -
This introduces a new 3,253-line production godfile and a 4,807-line test file.
gateway/slack_intake_ledger.pycurrently owns at least five separable responsibilities: correlation/key identity, private filesystem + WAL graph pinning, schema/migration/transactions, async admission/fallback health, and diagnostic snapshot/WAL parsing. The Hermes development contract's 2K invariant is a hard ceiling, not a target, and decomposition is still expected below it. Please split this before merge behind a narrow facade and split the test matrix along the same ownership boundaries. This is especially important here because the file is a security/reliability boundary: reviewability is part of the control, not cosmetics.
Topology / interlocks: I found no direct duplicate of this Ring 2 ledger. #62861 by irresi is complementary claim-lifecycle prior art: it fixes mark-before-success dedup ownership for WeCom and explicitly calls out Slack as a follow-up class; it does not collide with this PR's files. #85699 by jerryhjones is an older draft/non-mergeable conversational Kanban intake and does collide in plugins/platforms/slack/adapter.py; it owns a feature-specific durable intake source, not this transport-wide substrate, so it should be treated as adjacent/competing surface if revived rather than silently duplicated. #54535 by sk-holmes is also an older non-mergeable Slack adapter carrier; its read-only history work is distinct, but its profile/workspace isolation contract is directly relevant to the executor-scope finding above. #108914 already hit the same off-thread HERMES_HOME failure class in another subsystem and fixed it by carrying copy_context() into the worker. The repository-level architecture in #90866 describes the same defect shape: strong source ownership is established, then degraded at a projection edge before a persistence/mutation boundary consumes it.
Attribution: this head is one commit authored/committed by John Montgomery (m0ntydad0n). I did not find a co-author chain in the commit to preserve beyond that; the adjacent PRs above remain credited to their own contributors and should stay that way if any pieces are consolidated.
Verification state: the author's local receipts are substantial (392 passed focused; 980 passed across 43 files), and the branch is based exactly on current main. But exact-head hosted CI is not green: CI, Nix, and Docker are all action_required, the CI run exposes zero jobs, and there are no combined statuses on 160a46a. So those local results are useful development evidence, not an acceptance receipt for this exact object.
Once the profile context is preserved through the ledger worker, the Windows/platform boundary is closed, the two godfiles are decomposed, and exact-head hosted checks execute green, I think this is worth another pass. The core ownership model is promising; the remaining issues are all at boundaries where this layer has to be stricter than the code it is replacing.
| @@ -0,0 +1,3253 @@ | |||
| """Metadata-only durable Slack intake stage ledger. | |||
There was a problem hiding this comment.
Blocking: this lands a 3,253-line production godfile (with a 4,807-line sibling test file). The Hermes 2K invariant is a hard ceiling, and this module currently combines correlation/key identity, filesystem/WAL pinning, schema/migration/transactions, async admission/fallback health, and diagnostic snapshot parsing. Please split these ownership domains behind a narrow facade before merge, and split the test matrix along the same seams. For a persistence/security boundary, reviewability is part of the control.
| permits.release() | ||
|
|
||
| try: | ||
| future = executor.submit(work) |
There was a problem hiding this comment.
Blocking: this bare ThreadPoolExecutor.submit(work) drops the caller's ContextVars. _db_path() / _correlation_key() resolve get_hermes_home() inside this worker, while multiplex routing installs the selected profile through a context-local HERMES_HOME override. A secondary Slack profile can therefore persist into the process/default profile's ledger/key. Current main's tools.thread_context.propagate_context_to_thread exists for exactly this class. Capture the caller context before submit and add a two-profile regression proving routed profile B touches only B's ledger/key, never default A.
| connection lifetime removes that legitimate lifecycle race without adding | ||
| another filesystem member for strict diagnostic readers to encounter. | ||
| """ | ||
| if fcntl is None or not hasattr(fcntl, "flock"): |
There was a problem hiding this comment.
Blocking cross-platform boundary: every ledger transaction enters this lifecycle lock, so native Windows (no fcntl) can never use the durable path. _watch_opened_object() is also kqueue or Linux /proc+inotify only. Today Socket Mode therefore permanently degrades to process-local fallback and HTTP intake fails closed on Windows. Please provide a Windows-equivalent trusted serialization/object-identity path with exact tests, or explicitly narrow the supported-platform contract and docs; the PR currently claims general Slack intake durability.
160a46a to
b75a190
Compare
Summary
Reliability contract
Verification
scripts/run_tests.sh --file-retries 0 tests/gateway/test_slack_intake_ledger.py tests/gateway/test_slack_intake_adapter_observability.py tests/gateway/test_slack_intake_adapter_wiring.py -W error -q— 392 passed.scripts/run_tests.sh --file-retries 0 ... -q— 980 passed across 43 files at the canonical 28 workers.160a46aca91c4592ec2cf06c9a356e6fe05c217a.Local limitations
npm run checkand Ruff were unavailable in the local environment and were not counted as passing. No dependencies were installed to manufacture a green receipt.