feat(background): retrieve a completed RCA from chat and persist notify channels - #4974
Open
SawhneySatvik wants to merge 11 commits into
Open
feat(background): retrieve a completed RCA from chat and persist notify channels#4974SawhneySatvik wants to merge 11 commits into
SawhneySatvik wants to merge 11 commits into
Conversation
Contributor
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
Contributor
Greptile SummaryThe PR makes completed background investigations retrievable from chat and persists notification-channel preferences across shell restarts.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| config/constants/paths.py | Adds deployment_home() and centralizes organization-root ownership validation so scoped and unbound deployment callers resolve consistently. |
| platform/background_investigations/store.py | Moves the store to the deployment organization root and persists notification channels in the same locked, atomically rewritten document. |
| surfaces/interactive_shell/command_registry/background_cmds.py | Merges session and durable records and adds bounded, plain-text chat rendering with notification-error redaction. |
| surfaces/interactive_shell/session/background_investigations.py | Adds non-raising hydration of persisted notification preferences for new terminal sessions. |
| surfaces/interactive_shell/session/terminal_session.py | Initializes terminal notification preferences from the durable background-investigation store. |
| gateway/tests/runtime/test_slash_routing.py | Covers chat retrieval, plain-text formatting, bounded lists, unavailable shell-only commands, and redaction of persisted delivery errors. |
Sequence Diagram
sequenceDiagram
participant Shell
participant Store as Organization background store
participant Chat as Telegram/Slack/Discord
Shell->>Store: Persist completed RCA and notify channels
Chat->>Store: Read records via deployment_home()
Store-->>Chat: Completed RCA record
Chat-->>Chat: Render bounded plain-text response
Reviews (3): Last reviewed commit: "fix(background): reject /background use ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
13 tasks
SawhneySatvik
marked this pull request as draft
August 13, 2026 07:08
SawhneySatvik
force-pushed
the
feat/background-cross-surface-retrieval
branch
2 times, most recently
from
August 13, 2026 12:52
da3442c to
091eac7
Compare
tests/conftest.py redirects OPENSRE_HOME_DIR for the root suite. The gateway package has its own conftest and did not, so host_home() and opensre_home() resolved to the developer's actual home directory throughout gateway/tests. Verified before fixing: a probe test under gateway/tests/runtime reported OPENSRE_HOME_DIR = /Users/<me>/.opensre. Nothing read through those roots until now, which is why it never bit. The durable background-investigation store is the first reader, and the slash tests that assert "no background investigations" would have started failing on any machine where someone had run one real background investigation, while passing in clean CI. Patches the attribute rather than the OPENSRE_HOME environment variable: OPENSRE_HOME_DIR is resolved from the environment once at import, but both root helpers read the attribute per call. All 670 gateway tests pass unchanged, so nothing depended on the real home.
opensre_home() keeps an unbound caller on the host root so the CLI never writes into a customer's volume. That is right for almost everything and wrong for one artifact: a background investigation is started in the shell, which binds nothing, and retrieved from a chat transport, which binds the deployment's organization. The shell writes ~/.opensre and chat reads ~/.opensre/orgs/<org>/, so the record is invisible to the surface meant to retrieve it. deployment_home() resolves an unbound caller to the configured organization instead. A machine that names no organization has no second surface to share with and stays exactly where it was. The mount owner check moves to _org_root() and is now shared by both entry points rather than copied, so there is one tenancy policy rather than two that can drift. opensre_home() keeps its own contract unchanged, which the 18 pre-existing tests in this file pin. An unbound caller takes the organization id from the environment rather than from a validated principal, so it passes through the same _safe_segment check; a mutation that skipped it fails the hostile-id test.
The store resolved through opensre_home(), which keeps an unbound caller on the host root. The shell binds no storage scope and a chat transport binds the deployment's organization, so the shell wrote ~/.opensre/background and a Telegram turn read ~/.opensre/orgs/<org>/background. Retrieval from chat could never have found anything, whatever the reader did. deployment_home() puts both on the organization's root. A machine that names no organization has no second surface to share with and keeps the plain layout, which the second test pins. Also records in the module docstring that these are organization-owned rather than member-owned, so every member of a deployment can read every completed RCA. That follows the placement the issue asked for; session_home() is the per-member root. Called out here because this change is what makes the property observable. The contextvars copy in the runner is kept and its comment corrected: the record store no longer needs it, because deployment_home() falls back to the configured organization and a transport always binds that same one, but session transcripts, memory and integration reads on the worker still do.
The read forms answered only from session memory, which is empty on a chat session, so /background list and show from Telegram reported nothing whatever was on disk. _tracked_records merges the session dict with the store, session first: the runner persists only on the way out, so an in-flight record is fresher in memory, and only the live copy carries final_state. setdefault keeps that ordering, so REPL behaviour is unchanged. All four read sites use it, including use. Routing list and show but not use would let /background show succeed on an id that /background use then rejects as unknown. A store-only record instead reaches the existing "no completed RCA state yet" branch, because to_dict omits final_state by design. It cannot raise. dispatch_slash puts no try around the handler, so an unreadable document would reach the transport's generic error path and log a traceback for a readable condition. The message reports only the exception type: the store's own text carries the absolute document path, and unlike the REPL terminal a chat transport is an external sink. The empty-list message dropped "in this session", which is no longer what it means. Placed here rather than in terminal_access.py, whose docstring scopes it to the small slice of terminal state slash paths touch. Turning a pure attribute read into filesystem IO for every core caller is a larger change than this needs.
AC2 and AC3 land here. /background list and show now answer a chat transport with the RCA rather than a Rich table. Nothing on the chat path converts a table. print_repl_table renders normally into the recorded console, dispatch_slash captures it with export_text, and the sinks only package for transport, so the box-drawing glyphs arrive in Telegram as literal characters inside an 80-column hard-wrapped grid. The tests assert their absence, which is the load-bearing half: a content-only assertion passes against the table. show composes from summary_sections, the same bounded sections the chat notification adapters already send, so the reply respects the 4096-character cap those budgets were chosen for. list is bounded separately at ten rows with the root cause trimmed, because the transports tail-truncate and the closing "use /background show" hint would otherwise be the first thing lost. Both discovery loops close: the completion notice already carries the task id, and list now shows the ids too. Uses publish_headless_slash_response, the seam /onboard and /integrations setup already use, and prints as well as publishes so the captured-console fallback stays sane.
Channels chosen with /background notify set lived on the terminal facet and died with the shell, so a multi-channel setup had to be reconfigured every session. They persist in the record document under notify_channels rather than a second file. A separate file would need its own path resolver, its own lock, its own atomic write and its own tenancy rule, all of which have to match this one. Sharing the document is real coupling, so save() reads channels inside the lock it already holds and writes them back, and two tests pin that persisting a record and rotating past max_records both leave preferences intact. None and () are kept distinct: an explicit clear persists as an empty list rather than reading back as never-set, or turning notifications off would not stick. Hydration is what makes the write worth anything. Without it the document is write-only and the next shell still reads none; the terminal facet now builds its preferences from the store. load() never raises, because a damaged document must not stop the shell from starting. Persisting is reported rather than raised: the in-session change already took effect, so a write failure should say the setting will not outlive the session, not lose the turn. notify list and status fall back to the store on a chat session, which has no terminal facet and would otherwise answer "none" however the shell was configured. notify set stays shell-only: no acceptance criterion asks for it, and background mode is shell-only, so a chat-set preference would have no reader. Each half is pinned independently: removing hydration fails one test, removing the persist call fails both.
The page said background mode was session-local only and that jobs are not persisted across restarts. Both are now false: completed RCAs are kept and can be looked up from Telegram, Slack or Discord. The commands table gains a column saying which subcommands work from chat, since "which of these can I type at the bot" is the question a reader actually has. Adds the tenancy note. Completed RCAs belong to the organization, so anyone who can message the bot for it can read them, and that is worth knowing before investigating something sensitive. Says channels are remembered, which is the user-visible half of preferences being durable. Keeps the in-flight caveat, which is still true, and adds two limits that are real and were not stated: chat can look investigations up but not start one, and /background use needs the shell because the full state it promotes is not kept.
/background show became reachable from a chat transport in this branch, and a chat gateway message is an external surface under the CWE-209 rule while the local terminal is not. The notify row rendered the persisted result verbatim, so a "failed: <reason>" outcome carried exception text to a surface that must not see it. Collapses that form to "failed" on the headless path only. The REPL table is untouched, and the curated "missing <channel> integration: ..." hints stay whole because they are the actionable half.
SawhneySatvik
force-pushed
the
feat/background-cross-surface-retrieval
branch
from
August 14, 2026 05:45
091eac7 to
3ce5359
Compare
The resolver read the fifty newest rows while the store keeps a hundred, so a task id from the older half answered "unknown background task" even though the record was on disk. The completion notice hands the user that id, so coming back to an older RCA was the case most likely to hit it. list_recent gains an explicit no-limit form and the resolver uses it, rather than a second copy of the store's bound that could drift out of step again. The ten-row cap on chat listings is unrelated and stays.
The docs list use as shell only, but nothing enforced it. A chat turn fell through to the record lookup, and because the promoted investigation state is deliberately never persisted, it reported "unknown background task" for an id /background list had just shown. The guard now runs before the lookup and gives the same run-it-in-the-REPL answer the other write forms give.
SawhneySatvik
marked this pull request as ready for review
August 14, 2026 06:13
Contributor
Author
|
@greptile review |
Three docstrings led with shorthand from a private planning document, which means nothing to a reader of this repository. Each now states the behaviour it pins in the project's own vocabulary; the reasoning that was worth keeping is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4426
Builds on #4959, now merged, so this diff is only the second half.
Describe the changes you have made in this PR -
The second half you scoped: a completed RCA is retrievable from Telegram, Slack or Discord, and notify channels survive a restart.
Pointing
/background listat the store would have been a no-op. The REPL binds no storage scope and a chat turn binds an organization, soopensre_home()hands them different files and the list stays empty.deployment_home()resolves an unbound caller to the organization the deployment already serves, so both land on one document. The mount owner check moves into a shared_org_rootand still applies.Reads go through one helper, session records first, and chat surfaces answer in plain text via the existing
publish_headless_slash_responseseam. Preferences become a key in the record document rather than a second file, read inside the locksave()already holds so a record write cannot drop them.Worth your call: records resolve to the organization root, so every member of a deployment can read every completed RCA. That follows the placement asked for on the issue; say the word and it moves to
session_home().Duplicate-delivery suppression is deferred:
deliver()returns a discardedbool, so it needs the Protocol widened across nine adapters. No AC requires it.Demo/Screenshot for feature changes and bug fixes -
Verified across two real interpreters: an unbound writer, then an organization-bound Telegram-shaped reader whose replies came back free of box-drawing characters. Rich tables reach a transport as literal box-drawing text, so the tests assert their absence.
Full suite 15007 passed, plus lint, format, typecheck and
check-imports-strict. The one failure intests/cli/test_smoke.pyreproduces on cleanorigin/main.Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Two surfaces need one file. My first design read both roots and merged, guarded on whether the context mount was present. That guard cannot exist: "laptop plus gateway" and "one box, several organizations" set the same environment variables and resolve to the same path, so it is either on for both or off for both. It also gave a different-uid gateway
PermissionErroron every read, and the first save would have copied host records irreversibly into an organization document. Resolving one root for both surfaces removes all three instead of managing them._org_rootholds the mount owner check so there is one tenancy rule rather than two copies.deployment_homeis the unbound resolution._tracked_recordsmerges session and durable records, and_plain_status/_plain_list/_plain_showrender for transports.Checklist before requesting a review