Repository navigation
feat(mcp): propose isolated external tool support - #1732
Draft
JacobWeyer wants to merge 322 commits into
Draft
JacobWeyer wants to merge 322 commits into
JacobWeyer wants to merge 322 commits into
Conversation
# Conflicts: # docs/adr/0125-isolated-external-mcp.md
Stage configured complete sets under a separate pinned operator mTLS role. Retain every managed session deny union before host I/O, require exact source readiness, and publish only after the durable final barrier. Add paired store races, scanner recovery, and actual TLS authorization coverage.
Remove exact whole-schema duplicates during grant intersection. Keep distinct schema roots and preserve stored binding documents and hashes. Review: corrected a stale host-enforcement comment and made the stale-profile test change a real constraint. Deep-child and stored-binding regressions pass.
The durable cancellation fence already blocks delayed admission. Do not stall the ordered consumer retrying an absent execution forever. Review: existing-row identity and authority failures remain retryable. The three cancellation composition regressions pass.
Use the existing disposable managed control stream for private handoff closure so a draining session cannot hold the shared stream. Review: checked the neighboring managed notifications and retained ordinary stream behavior. The stalled-handoff regression and boundary tests pass.
Prepare the listener, parsed TLS configuration and caller pins before advertising the host. Serve from that one prepared value. Review: removed duplicate configuration reads and checked startup failure cleanup. Invalid PEM, occupied listener and pinned serving tests pass.
Deduplicate external calls by exact call ID while retaining the native question and plan workaround. Keep strict configuration required even with no approved external tools. Review: removed unused optional configuration state and checked launch arguments. Deferred-call and strict zero-tool regressions pass.
Keep compact completion receipts in the existing session store. Persist a stable late-result follow-up ID before sending and reconcile accepted native history on restart. Never replay the external effect. Review: closed the acceptance-before-receipt crash gap and kept cancellation and plan isolation. Restart and delivery regressions pass. Receipts remain for the session lifetime because no host completion acknowledgement exists.
Use the pinned SDK termination operation before local transport close, with the exact endpoint, session and credential and a bounded best-effort deadline. Preserve a known execution result if teardown fails. Review: checked credential and destination binding, modern stateless behavior and cancellation. Full worker tests pass. Format the restricted-fetch type signature.
Honor capture acknowledgement and authorized destroy through explicit file-retention state. Propagate stop failures and require stopped native attachments before releasing source files. Review: kept unknown starts and cleanup failures conservative and checked repeated close paths. The VZ library and targeted host checks pass; physical qualification remains separate.
Persist an observed provider account in the sealed bundle, retain it when the optional field is omitted, and reject a later account change. Review: preserved scope, credential identity and rotating-refresh CAS fences. The observed-account sequence and OAuth boundary tests pass.
Distinguish storage, migration and serialization failures from stale authority. Keep public errors static. Review: checked all metadata error variants and the public control mapping. The precise classification regression passes.
Supply the required owned thin-pool input in the positive fixture. Assert precise failure reasons for missing or invalid pool inputs and existing negative cases. Review: checked the real CI command and retained root-required coverage as an explicit skip locally. The writer suite passes.
Retain target identity before deletion and require completed destroy metadata, no pending operations or intents, and no exact tombstone before deleting test ownership. Apply the same barrier to children. Require prerequisite evidence in writer and replay. Review: removed parent cleanup after a failed child barrier and checked fixed read-only query scope. All 215 script tests pass. Final physical retirement proof remains mandatory.
macOS can return EPERM for a group containing only unreaped zombies. Keep it unresolved within the existing bounded wait; only actual absence plus leader exit permits cleanup success. Persistent denial still fails. Review: checked exact child signal scope, mock restoration and unchanged cleanup deadlines. Three new regressions and the complete 190-test platform suite pass.
Use the existing managed digest predicate instead of maintaining an identical copy. Review: compared both predicate bodies and checked callers. Existing managed creation tests pass.
Describe bundled policy installation, current authority and original-destination handling, the registered OAuth callback, and actual fixture prerequisites. Review: removed stale transport and historical incident text and checked the effective deployment and test contracts. Runtime Rust changes in this commit are comments only; JSON and format checks pass.
Remove only the historical boot-to-host foreign key in a forward migration. Preserve all immutable boot, capture and intent identity. Active authority still requires current placement and a usable host. Review: checked native terminal host deletion and stale-claim error precedence. Paired simulator/Postgres history and absent-host claims pass; no applied migration is edited.
Lock placement hosts in sorted order before policy parent sessions. Use NO KEY UPDATE consistently for capacity locks so ordinary teleport and rebind foreign-key checks can complete. Teleport admission selects only hosts actually prelocked. Review: a complete source read exposed the ordinary policy-parent cases. Both real deadlocks were reproduced; bounded shared regressions cover admission, commit under a held child host lock, capacity and refusal precedence. No per-method commit/rebind lock framework is added; native retirement locks remain stronger.
Return the same Conflict variant as Postgres for absent MCP OAuth flow state. Review: compared both store implementations and added the precise shared assertion. Both conformance variants pass.
Read assigned nonterminal capture jobs from the existing simulator table and return the current host assignment epoch. Exclude other hosts, unplaced jobs and terminal jobs. Review: the full-file pass found the old empty stub and stale neighboring comments. The shared scenario failed only Sim before the fix and now passes against both stores. Independent source review passed; temporary test databases were removed.
Integrate the single HostHeadroom calculation and reservation SQL, the queue-aware parking policy, and bounded Firecracker handler diagnostics. Preserve managed reservation accounting, sorted NO KEY UPDATE placement locks and the admission prelocked-host restriction. Review: the capacity merge exposed a measured-CPU fallback inconsistency. Use the shared independent-dimension fit rule in preview, transactions and diagnostics. Add the mandatory managed fixture fields. Independent source review passes; 64 coordinator/core and 18 paired persistence tests pass. Full merged-source and target-specific gates follow this merge.
Idle nomination marked a managed session Evicting before ordinary lifecycle dispatch rejected it. Apply the existing managed authority gate before nomination and direct descent so a Started session retains its active state. Hold state when the authority read fails. Review: reuse one gate and propagate ApiError through the private descent caller instead of relabeling read failures. The real simulator boot fixture proves no operation, event, rung or host effect; the ordinary control still succeeds. All 64 focused coordinator/core tests pass after three intended failures against the old behavior.
Preserve a genuine host stack-overflow RangeError and stop native access to the failed fresh WASM instance. Keep log handles under the outer owner and dispose live handles, context and runtime in order for normal guest outcomes. Pre-commit review found that the SDK consumes Promise error handles. Skip consumed handles while stopping at the first actual cleanup failure. Independent review passed. All 15 original tests and seven resource limits remain unchanged. Validation: the new compiled-tier regression failed before the repair. Focused checks pass, and the complete default and compiled-tier suites each pass 3313 tests with 18 existing skips. Typecheck and formatting pass.
Read exact resource endpoints from operator configuration and OAuth registrations for each setup request. Reuse live publication and worker checks while retaining credential status and recovery when publication is unavailable. Existing configuration remains readable. Pre-commit review found that publication failure could hide recovery controls. The repair now returns no choices while keeping recovery metadata available. Independent re-review passed; all eight touched files received a separate full review. Validation: the real runtime-to-RPC regression failed before the fix. All 37 focused tests, typecheck and formatting pass. The complete local suite passes 3314 tests with 18 existing skips and zero failures.
Pass the authenticated ForgeSink to the managed boot owner. Register the credential port before either backend starts the harness, and keep the Firecracker listener in the boot owner's task set. Exercise the framed credential request and listener shutdown. Verify that the coordinator rejects a broker token from another session. The separate pre-commit review found stale comments in the host and API tests. Their cleanup is separate from this behavior change.
Describe the current RAM snapshot and heartbeat retry behavior. Attach the shutdown signal documentation to the signal handler. Remove stale references to test coverage. The separate review confirmed that every changed line is a comment.
Fill the missing host proxy CA before managed SpawnHarness. Use the same host-owned helper in both start paths and preserve an explicit CA. Agentd installs the CA before it starts the harness. The regression covers managed dispatch, an explicit CA, and repeat start. The separate review found two stale comments for cleanup.
Describe the stage duration metric at its recorder and state the snapshot lease requirement at the keepalive. The review found no behavior changes in these two comment corrections.
Keep the official helper executable at the same bundle level as Codex. Restore its mode after CI downloads the archive and restage an existing ProcessBackend bundle when the sibling is absent. The separate review checked both staging paths and the workflow. It found the same-version ProcessBackend reuse case covered here.
Use the shared 32 MiB host RPC limit while preserving managed TLS pins and protocol checks. Keep the upstream large-message regression and the existing wire compatibility and cancellation tests. Supply the managed fields in the ordinary session-budget fixture. Its assertions and the Postgres and simulator conformance cases stay intact.
Author
|
@nikhilunni I had success running all of engrams locally with the MCP server work and using a remote AWS firecracker VM as well! I do not have a k8s cluster I can do full testing in; but after over a week of back and forth this seems to be in a good place. I've added a proposed way to break up this stack of changes into 6 independent PR's but please tell me what would make considering this easiest to review? |
This branch has not been deployed
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
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.
Problem
Claude and Codex sandboxes need organization-managed external MCP tools. The guest must not receive organization credentials or decide which remote service, account, or tool it may use.
This disabled implementation is open for architecture and scope feedback. The contribution guide asks for an issue before a large change; the design discussion can move to an issue if preferred.
Fix
Remote HTTPS MCP calls use an isolated worker and the existing connection, credential, profile-grant, and
ENGRAM_TOOLSmodels.flowchart LR S[Claude or Codex sandbox] --> C[Coordinator] C --> O[Orchestrator: grants, approvals, journal] O -->|mTLS and bound intent| W[Isolated MCP worker] W -->|HTTPS and organization credential| M[Approved MCP server]Reviewed immutable catalogs and session bindings limit exposed tools. Restricted transport validates destinations and keeps organization credentials outside guests. Interactive writes require bound approvals; unattended writes need explicit administrator grants. A durable dispatch barrier prevents automatic replay of uncertain writes. Both harnesses preserve text, structured JSON, and MCP error results.
Managed boot and disk-only recovery keep the guest closed until the exact owner and network policy are verified. Direct guest access to managed service authorities is denied. Ordinary checkpoint, eviction, resume, and migration remain closed for managed sessions; a lost native owner is not adopted.
The feature stays disabled by default. Guest stdio servers, package installation, personal credentials, resources, prompts, sampling, elicitation, MCP Apps, and asynchronous MCP Tasks are outside this release. Deterministic MCP and OAuth services are the acceptance targets.
Suggested review units:
Keep each boundary with its contracts, migrations, and tests. A scope and simplification review has already removed unused credential mutators and duplicate CI execution. This change adds one full Drizzle snapshot; required migration history stays intact. No applied migration is edited, and there are no SQLx cache changes relative to the base.
Proposed PR split
Keep this draft as the integration reference. The first decision is whether the managed networking, boot, and storage changes belong in the initial MCP release. If maintainers prefer to start with public HTTPS servers and reviewed read tools, check which platform changes that scope still needs to keep organization credentials outside guests and block direct guest access to managed endpoints.
The QuickJS recursion fix and the Codex helper packaging fix are candidates for separate PRs from
main. Before opening either one, confirm that it applies independently and passes its focused regression test.Worker review can start in parallel with the platform work once reviewers agree on the shared contracts. Each PR should carry the tests and generated files for its changes. Keep database metadata with the migrations it describes. Regenerate intermediate snapshots where a split changes the schema boundary. Run physical acceptance on the final combined commit.
Feedback requested. Is this the right platform boundary for upstream? Would a first release for public HTTPS servers with reviewed read tools be a better starting point?
Upstream integration
The published head is signed merge
db5cbf05, which includes upstreammainatbc1b1e76. The three conflict resolutions keep the shared 32 MiB host RPC limit, managed TLS and protocol checks, and both sets of gRPC tests. The new ordinary session-budget fixture supplies the managed fields with empty values and keeps the upstream assertions. The merge also includes the upstream idle parking, recorded resume budget, and first peer dial fixes. The earlier published head3f723ed4added the signed QuickJS containment repair and includes upstream1f9266d9through signed merge5e6c52fe. The managed idle guard is the separate signed commitdf0c19f5; the earlier complete review repair series ends at6e128c6d. GitHub reports the current draft as mergeable. Review and CI remain open.The merge uses upstream's shared FillEngine, session-disk flow, HostHeadroom calculation and reservation SQL. The later queue-aware parking update is preserved. Known CPU capacity still gates when RAM is unmeasured. Managed idle nomination and direct descent use the existing authority gate before any session-state change. Managed cold disks are fully materialized before network and VM creation; protected managed volumes do not enter ordinary settle drivers. The host binds its listener before registration and preserves pinned managed TLS on that listener. Removed upstream manifest and test-fixture APIs have been updated. Materializer layout version 2 records the changed managed-DNS init bytes and keeps an exact packed-image fingerprint; existing stored disks are preserved. Complete manifest equality covers lost-reply reconciliation, including chunks and annotations.
Ordinary sessions retain wire version 38 and source-pinned predecessor compatibility tests. All eight additive managed host RPCs separately require managed protocol 43 and pinned mTLS. No peer-version negotiation or plaintext managed fallback was added. ADR 0127 records the trust boundary and deferred scope.
Adversarial review
The pre-merge review covered 634 changed paths and recorded 36 confirmed findings fixed. Separate reviews covered the current gRPC conflict resolutions, idle and session budget behavior, Postgres and simulator conformance, and peer migration. The scopes overlap. Coverage distinguishes full source reads, targeted changed-code review, and generated-file verification. Independent reviews checked the repairs and cross-component invariants; focused follow-ups checked defects found during validation. Large persistence and simulator files also received the repository-required full pre-commit read.
Repairs include current administrator authorization under the publication lock, OAuth account identity retained across refresh, exact external call identity in both harnesses, durable Codex completion recovery, legacy upstream session termination, host TLS preparation before registration, private handoff stream isolation, native VZ file retirement, and cancellation before journal admission. Test cleanup retains its exact target and ownership until recorded obligations finish, and final physical proof is still required.
The persistence changes keep host history without blocking native host deregistration, use host-before-session locking for managed placement and ordinary policy-parent teleport, and align simulator capture assignments with Postgres. Shared capacity locks permit foreign-key key checks while still serializing writers and deletion, consistent with PostgreSQL row-lock rules. A new forward migration removes only the historical host foreign key; no applied migration is edited and no history is deleted. Shared publisher and digest helpers replace duplicate rules. Exact repeated schema documents no longer grow at each child level; stored binding hashes stay unchanged. Migration journals and the one new full Drizzle snapshot remain required history.
One design question remains for activation: a missing reserved capture manifest stops shared chunk garbage collection. This is the intended data-preservation rule; operational qualification remains open. Safe abandoned-capture cleanup must be qualified before enabling managed sessions. Codex completion receipts also remain for the session lifetime because the current protocol has no durable host completion acknowledgement.
Test
Latest merge checks
just checkpassed formatting, workspace/all-target Clippy and Hakari drift checks. Nextest reported 3,456 passed, 389 skipped, zero failed.Earlier local checks retain the following source scope.
35742e45passed formatting, workspace/all-target Clippy and Hakari drift. Nextest recorded 3,450 passed, 388 skipped, zero failed.35742e45: passed.3f723ed4: typecheck and formatting passed. The complete unfiltered suite passed twice: 3,313 passed, 18 skipped, zero failed, across 304 files, both with normal Bun settings and with the compiled WebAssembly tier that reproduced the recursion failure. All 15 original sandbox test bodies and all seven resource limits are unchanged. One bounded subprocess regression adds six cases, including healthy next evaluations and normal Promise-error disposal. It failed against the old code. Each full run used two new identity-verified databases; all were removed, with no test descendants left.03da8d5c: formatting, typecheck, and 3,314 passed, 18 skipped, zero failed across 304 files. The new regression covers the runtime-to-RPC setup destination lookup.In the earlier review, the first local Rust test attempt failed before AWS fixture calls because macOS native root discovery returned no certificates. The supported
SSL_CERT_FILEsetting with the unchanged operating-system PEM bundle restored the 13 AWS secret tests. Certificate verification stayed enabled. That earlier source-matched workspace run reached every selected test. For the current check, an initial attempt lacked Hakari on PATH. A second picked up Node 25 and the two worker-backed Rust tests failed the worker startup requirement. The final run used Node 24 and reached every selected test.Local UI exercise and broker repair at
1b8dd4b1The browser UI setup completed on signed source
03da8d5c: an administrator enabled and staged the guest image, connected genuine Codex device authorization, completed fixture OAuth, discovered and reviewed two tools, published catalog revision 1, enabled the connection, and saved constrained profile grants.Commit
8de1be5eattaches the existing authenticated credential broker to the managed Firecracker and VZ boot paths before the harness starts. A native VZ session reached Codex account discovery after obtaining its OAuth credential through the broker. A control session using the same direct Codex model and credential on ordinary VZ returned the expected response.An unauthenticated HTTPS probe inside managed VZ failed certificate verification. Commit
42a9cebamakes both host start paths supply the same public proxy CA before the harness starts. The managed guest's installed CA matched the host CA and the real Codex model then attempted its first tool call.Commit
1b8dd4b1stages the official code-mode helper beside the CLI. The CI artifact path restores executable mode and existing ProcessBackend bundles restage if the helper is missing. The staged x86_64 package and packed aarch64 bundle retained the official executable bytes.The browser UI test then passed in a fresh native VZ session with genuine Codex and the reviewed local OAuth MCP fixture. The model read count 0, requested one delta-one write, waited for an owner decision through the UI, completed that write, and read count 1. The authoritative journal contains three successful calls, one write, one consumed approval, text and structured results, and zero unknown outcomes. The successful VM ended through the normal session lifecycle. Physical cleanup confirmed zero live sessions, zero running sandboxes, no managed VM directory, and no helper process. The local app and saved test configuration remain available.
This test uses a real model and a deterministic MCP provider. The current-source full physical matrix and upstream CI gates remain open. Two local retries stopped in the private-network helper before the CLI launched while Rust checks were running. The successful final session ran after those checks finished. These failed attempts remain in the local evidence.
Physical evidence is source-specific:
c5dc8e7bb75b0d7fdb5cbf05The Firecracker terminal verifier returned
FULL_24_PASS. Matrix checks, pinned physical reads, boot coverage, native-process closure, final pool metadata ownership and cleanup passed. The final archive and selected member hashes were verified locally. All 28 build pins were retained. The temporary cloud instance and all owned disks, network, access and expiry resources were removed and their absence verified. No new cloud runner was created for this review.That run includes the scanner-driven rebind repair: the earlier three revocation/OAuth recovery failures all passed in the coherent 24-case run. Test assertions were not weakened or omitted. The historical sandbox recursion failure is retained in the baseline document and is now repaired as described above. Earlier storage and protocol checks retain their original source scope. The new local web result above is source-matched; none of these checks is a current-head upstream CI result.
QuickJS recursion repair
Bun can exhaust its host WebAssembly stack before QuickJS reaches its separate shadow-stack guard. The wrapper now preserves that genuine host RangeError and makes no more native calls into the failed fresh instance. Host garbage collection reclaims the instance; reclamation is not immediate. Other host failures remain SandboxError. Normal guest outcomes still release live handles, context and runtime in order. Independent review caught and closed a consumed Promise-handle cleanup defect in the repair. No dependency, production JIT setting, resource limit or existing assertion changed. The new regression requires empty child stdout and stderr, so native cleanup assertions remain failures.
Remaining CI and release gates
The current CI run reports
action_requiredwith zero jobs started. An upstream maintainer must approve the fork workflow. Both physical lanes remain required forCI Gateon the current merge.The workflow requires dedicated self-hosted runners:
self-hosted, Linux, X64, engram-mcp-firecracker.self-hosted, macOS, ARM64, engram-mcp-vz.The runner operator must install the documented configuration, qualified storage/network paths, and pinned artifact access. This account receives HTTP 403 from the upstream runner-list API, so registration and availability cannot be confirmed. Fork workflow approval and runner access require upstream maintainer/operator authority. Manual acceptance does not satisfy these CI jobs.
The reviewed changes affect shared host, storage and harness behavior, so both physical jobs must qualify the new source. They remain dependencies of
CI Gate; no checks were removed or bypassed.Mixed-build deployment, managed activation, upgrade/recovery, rollback, proxy replacement with VM survival and abandoned-capture cleanup remain separate release gates. Source-level predecessor compatibility does not prove a deployed mixed fleet. The PR remains draft and the feature remains disabled.