Refactor context engine boundaries and daemon reconciliation - #1057
Conversation
Establish accepted ownership contracts for the importable Context Engine, Potpie resource management, the daemon, and CLI human/machine boundaries. Record typed daemon, context isolation, explicit composition, destructive intent, and deferred parsing, extension, and protocol decisions without claiming implementation conformance.
|
Important Review skippedToo many files! This PR contains 550 files, which is 250 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (550)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b692611ba
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Authentication scope follow-up remains intentionally separate from this remediation. OQ-AUTH-MODEL-001 is tracked in spec/questions/open.md and covers local IPC, Potpie login, hosted clients, integrations, and browser access as distinct trust boundaries. This PR keeps the bearer credential documented as transitional daemon authentication; it does not expose that bearer to browser JavaScript or claim browser/UI authentication is solved. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Two follow-up findings (low severity, but they conflict with accepted spec behaviors)5. On a shared machine, PID liveness probing itself is unspecified in the contract, but this runs against two accepted behaviors: DAEMON-055 (artifacts may only be removed while holding the runtime ownership lock) and DAEMON-056 ( 7.
This doesn't match either accepted error category: CLI failure summary ( (Findings 4 — port-selection TOCTOU — and 6 — |
|
Thanks @ASCE-D. Both follow-ups are addressed in b99ad4d. For For Verification:
|
ASCE-D
left a comment
ASCE-D
left a comment
There was a problem hiding this comment.
Approving. My earlier findings are resolved and verified on the latest head: #2/#3 fixed in e301c9c (resolved pot_id now dispatched as an explicit selector; snapshot paths absolutized CLI-side), #5/#7 fixed in b99ad4d (PermissionError treated as alive in _pid_alive; CLI-side subgraph validation fails without dispatch). Targeted test suites pass locally (81 tests). The unauthenticated loopback UI API is intentionally deferred per the author; the port-selection TOCTOU and .env-loading semantics remain as unspecified spec gaps worth an OQ entry or follow-up issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b99ad4d153
ℹ️ 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".
| write_daemon_pid(self.home, outcome.value.pid) | ||
| write_daemon_discovery(self.home, discovery) |
There was a problem hiding this comment.
If the parent CLI is killed after the authenticated readiness handshake but before these writes, the child remains live while holding daemon.runtime.lock, but only its credential exists: status reports it as down, stop has no PID to authenticate, and a subsequent start is rejected because the live child owns the lock. Fresh evidence after the claimed fix is that the child callback in potpie/daemon/__main__.py still writes only the credential, while PID and discovery publication remains parent-owned here; publish the complete identity from the lock-owning child before readiness.
Useful? React with 👍 / 👎.
| try: | ||
| engine_outcome = await self._engine_factory( | ||
| context=context, | ||
| config=composition.config, | ||
| dependencies=composition.dependencies, | ||
| ) | ||
| except Exception as exc: |
There was a problem hiding this comment.
If an acquisition task is cancelled while the engine factory is awaiting, asyncio.CancelledError bypasses this except Exception block after compose() has already returned its host resources. The engine is never cached, so manager shutdown cannot discover or release those connections or handles; cancellation should run the failed-acquisition cleanup before being propagated.
Useful? React with 👍 / 👎.
| validation_error = response_validation_error(request, response) | ||
| if validation_error is not None: | ||
| return Failure(validation_error) |
There was a problem hiding this comment.
When a mutating request has executed but its otherwise valid response carries a mismatched request ID or protocol version, this returns a plain ProtocolError with no dispatched or unknown-outcome marker. Callers therefore cannot distinguish the response defect from a pre-dispatch failure and may retry an already-applied mutation; correlation validation failures for dispatched mutations need the same unknown-outcome treatment as schema-invalid responses.
Useful? React with 👍 / 👎.
| async def _call(self, call: Callable[[], object]) -> Outcome[object]: | ||
| try: | ||
| return Success(await asyncio.to_thread(call)) | ||
| except CapabilityNotImplemented as exc: |
There was a problem hiding this comment.
If a local or daemon operation is cancelled while this to_thread call is executing, cancellation stops the await but does not stop the synchronous backend function. The handler then unwinds the coordinator context and releases its lease while the worker can still be mutating the graph, so a subsequent same-context operation or shutdown can run concurrently with it; shield the worker and wait for its completion before propagating cancellation.
Useful? React with 👍 / 👎.
| def get_runtime(): | ||
| """Return the process-wide explicit runtime composition, built lazily.""" | ||
| if _state["runtime"] is None: | ||
| from potpie.runtime.composition import build_local_runtime | ||
|
|
||
| _state["runtime"] = build_local_runtime() |
There was a problem hiding this comment.
Root-owned commands such as config list, config set, and pot/source administration now reach this full build_local_runtime() call through get_root_runtime(), even in daemon mode. If the CLI environment selects an invalid or unavailable graph backend while an existing daemon was started with a working profile, these metadata and recovery commands fail during local backend construction before reaching their independent services; compose the root capability group without constructing the graph runtime.
Useful? React with 👍 / 👎.
| ) | ||
| if spec.safety is SafetyClass.SHARED_CONTEXT_READ_EXCLUSIVE_RESOURCE_WRITE: |
There was a problem hiding this comment.
With the supported in_memory backend, different pots still share one InMemoryClaimQueryStore, but every mutation receives only its per-context write lock here. Concurrent operations on different pots can therefore race on the same lists and indexes; for example, reset_pot() replaces store.rows while a mutation for another pot appends to the previous list, losing the latter update. Give this backend a shared resource lock or make its store operations internally synchronized while retaining concurrency for backends that support it.
Useful? React with 👍 / 👎.
…ntime Main's #1057 dissolved potpie-context-core into potpie_context_engine.core and replaced HostShell with LocalRuntimeComposition. This merge re-homes the document-ingestion feature accordingly: - resource contracts + ports moved to potpie_context_engine/core/ (imports rewritten repo-wide; the potpie-context-core package is gone) - ResourceService is wired into LocalEngineServices.resources by build_local_runtime, chunk_search wiring preserved; document/resource CLI commands use get_runtime()/get_root_runtime() instead of HostShell - removed an accidental nested duplicate of the potpie-document-ingestion skill (potpie-document-ingestion/potpie-document-ingestion/) that had been failing the snippet linter; updated the skill's SKILL.md/examples.md/ reference.md and claude_bundle/CLAUDE.md for main's template rules (potpie ingest and context_record are gone -> potpie record) - characterization pins extended deliberately: LocalEngineServices gains 'resources'; CLI surface gains 'document' and 'resource'; skill catalog gains potpie-document-ingestion Root suite: 1474 passed, 0 failed. Engine suite: 1224 passed; 3 pre-existing failures (docling fixture x2, pdf_docling patch-target drift). CLI verified end-to-end: ingest, search, show, agent-path resource import, list, rm.
2 participants
Performs a Context Runtime Boundary migration: one importable context-bound engine, one Potpie-owned resource manager, one typed local/daemon execution contract, and one canonical foreground daemon launched with
python -m potpie.daemon.This PR replaces the reflective
HostShell/RemoteHostShellarchitecture and removes the standalone Context Core boundary. It preserves existing CLI JSON envelopes and exit categories, with the approved addition of explicit confirmation for destructive graph operations.Architecture
ContextEnginefacade with typed requests, results, outcomes, and idempotent lifecycle.ContextResourceManagerandAuthorizedContextLeasefor selection, authentication, authorization, dependency ownership, caching, and deterministic shutdown.LocalEngineClientandDaemonEngineClientoperations.python -m potpie.daemonprocess.--yes, and a human decline is returned as a typed local cancellation.Removed legacy architecture
/rpcand/attrroutes and Python-class wire codecs.RemoteHostShell,RemoteSurface, and the reflective daemon client/server.HostShell,build_host_shell,get_host,set_host, temporary runtime adapters, and the migration allowlist.potpie-context-corepackage and dependencies.Permanent AST/TOML architecture tests prevent these symbols, imports, routes, packages, and entrypoints from returning.
Review guide
The commits are intentionally ordered as independently reviewable migration slices: