feat(sandbox): run Routers and Handlers in the subprocess sandbox by default (BACKLOG #1278) - #879
feat(sandbox): run Routers and Handlers in the subprocess sandbox by default (BACKLOG #1278)#879wshallwshall wants to merge 5 commits into
Conversation
…default (BACKLOG #1278) `[sandbox].mode` shipped "off", so on the shipped default a Router or Handler body ran in the engine's own address space, next to the DEK, the audit chain and every connection's in-flight data. The isolation mode has been built and exercised since ADR 0087. Only the default was wrong. This flips it to "subprocess". State the limit first, because a reader must not buy a boundary that is not there: this does NOT stop config Python executing in the engine process. The loader still runs every *.py in the config directory in-process, as the service account, at every serve and every reload, ungated by mode. What mode governs is where a Router's or Handler's body runs once the graph is built. mode=off is untouched and stays supported. It is the escape for a Handler that needs the live db_lookup/fhir_lookup bridges, which fail closed inside the child because they re-enter the event loop. The docstrings now carry the costs ADR 0087 already measured, rather than re-deriving them: the refusal of live enrichment, the wall cap that starts being enforced (there is no timeout at all at mode=off), the per-dispatch throughput and the fact that one message is not one dispatch, the per-inbound process and handle multiplier, and the pre-deploy gate's blindness to the setting. SandboxPolicy.mode loses its `= SandboxMode.OFF`. A second default on the dataclass would have been free to contradict the settings default silently; all 12 construction sites already pass mode= explicitly, so requiring it costs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…flip changes (BACKLOG #1278) Three pins asserted the old default and two knobs now read wrong on their own page. tests/test_threat_model_doc_drift.py pinned `s.sandbox.mode == "off"` under the label "no Router/Handler wall cap". That label is backwards after the flip: at mode=off `run_sandboxed` is `fn(payload)` with no timeout at all, and it is the subprocess default that arms wall_seconds. The shipped posture gained a cap it did not have. The `_DANGEROUS_ROW_KEYS` anchor naming "In-process (default)" now names vault text that is false, and is deliberately NOT re-picked: choosing a replacement from a checkout that cannot read the vault document would trade a stale anchor for an unverifiable one and red the leg for whoever does hold it. Both are recorded as coupled vault-side edits, the same way the pickle anchor already is. tests/test_phi_logging_inventory.py pinned the same value to say "revisit §7". The reason strengthened rather than lapsed, so the pin flips and says why: the child-stderr relay §7 documents is now the default logging path. checks.py advertised that the gate "previews what the default engine actually delivers". That claim is scoped to snapshot_on_send and stays true, but a reader can generalize it, so both it and dry_run's own docstring now state the limit outright. dry_run takes no `sandbox` argument and always runs in-process, so a Handler calling db_lookup/fhir_lookup passes the pre-deploy gate green and then fails closed at serve, and wall_seconds is unenforced in the preview. Stated rather than fixed on purpose: route_only/transform_one already take `sandbox=`, so teaching the gate to spawn a worker child per inbound is its own change with its own cost, not a side effect of a default flip. PipelineSettings.fuse_thread_hops said nothing about the interaction, though the runner hard-disables fusion when both are set. An operator reads the knob they set, not the other one, so the warning goes on fuse_thread_hops. tests/test_settings.py needed no edit: its round-trip arm already picks the opposite of whatever the current default is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n (BACKLOG #1278) Each of these asserted a default that changed, and two of them used it as load-bearing reasoning rather than description. CONFIGURATION.md gets the operator-facing rewrite. It now leads with the limit -- the loader executes every *.py in the config dir in-process regardless of mode -- then tables the four costs of the default (refused live enrichment, an enforced wall cap where there was no timeout at all, throughput with the one-message-is-not-one-dispatch multiplier, and the per-inbound process cost), then names the two things the setting does not reach: the pre-deploy gate, and fuse_thread_hops, which the runner hard-disables underneath it. ADR 0087 is AMENDED, not rewritten. The status line records the flip, the Decision keeps its original opt-in wording with the change called out beneath it, AC-1's "(the default)" becomes "the default as decided; the opt-out since #1278", and the Consequences line claiming default-off buys byte-identical behaviour for existing deployments is corrected, since that is now false of what ships. ADR 0144 is the one where the fact was doing work. Its rejected-alternatives rationale -- the stated reason the static lint gate exists at all -- gave two grounds for refusing to rely on the sandbox alone, and "it is off by default" was one of them. The rejection still holds on the other ground, which is sufficient by itself: an address-space boundary does not catch a Handler leaking PHI into the store's own log or building SQL inside the sanctioned db_lookup. Neither crosses an address space. Said that way, so a reader can see the rejection survives. ASVS-ASSESSMENT-METHOD.md used this exact cell as its worked example of rule 5, "a working control that ships off". The premise expired. The FACT is corrected and the `partial` verdict is deliberately LEFT ALONE -- re-scoring a cell is the tracking seat's act against the vault, not a Builder's. The table now says no live worked example of rule 5 remains, rather than inventing a substitute, which is the same choice the 3.7.3 row already made and for the reason it gives. PHI.md §7 notes the scope change: the child-stderr relay it documents is now the ordinary path, so both covering mechanisms are load-bearing on the shipped default. DEPLOYMENT.md, ADR 0147 and the ADR index carry the same correction, and the index's "default-off" residual is struck as closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntradict it (BACKLOG #1278) A default flip is only real if it survives the seam it travels through, and three separate objects could each have contradicted it while a test on any one of them alone still passed. SandboxSettings.mode is the setting, and the only place a default belongs. Engine(sandbox_settings=None) is the caller that passes nothing, which before this change meant in-process. SandboxPolicy used to carry mode = SandboxMode.OFF as a second, independent default; it now carries none, and the test pins that with a dataclasses.MISSING check rather than trusting the constructor. That assertion was falsified against wall_seconds, which does have a default and reads as not-MISSING, so it discriminates. The test also pins that mode=off is RETAINED, because removing it would strand every Handler needing db_lookup or fhir_lookup -- those fail closed inside the child, and mode=off is their supported escape. Deliberately not a subprocess round-trip. Parity and isolation are already covered by the eleven groups above it; this asserts only which mode a stock engine resolves to, which is the part that changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the item open The flip landed and the row's named evidence gap is closed a second time, this time ON the flipped default. The banner carries the paired A/B against a mode=off control -- same ACKs, same end-to-end deliveries, 11 worker children versus 0 -- so the instrument is shown to discriminate rather than asserted to. The winning arm declared no [sandbox] section at all, which is what proves the default alone carries it. One counting trap is recorded because it would otherwise be reported as a finding. The raw process count was 22, not 11: on this box the venv python.exe is a redirector that re-executes with a byte-identical command line, so every logical process appears twice under the same CommandLine. Read naively that looks like workers spawning workers and contradicts finding 2. It does not. The two OB_IMMUNIZATION_* start failures are shown pre-existing by appearing identically in both arms. The item STAYS OPEN, narrowly and explicitly, for two coupled rows in the vault-only THREAT-MODEL.md that no checkout can see: the 15.1.3 row, which must now say the wall cap is enforced on the default rather than off it, and the _DANGEROUS_ROW_KEYS anchor naming "In-process (default)". The anchor was deliberately not re-picked from a checkout that cannot read the document -- that trades a stale anchor for an unverifiable one and reds the leg for whoever holds the vault. The prior lane's re-scoring signal is confirmed: difficulty 3 was wrong, 5-6 is right. The dispatch note's full-suite demand was met differently rather than ignored, and which legs ran locally versus which are CI's job is written down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed: read the settings change, the regression test, the A/B evidence and the affected-docs sweep. Five commits, The control is what makes this landableI briefed the item's own capitals at this Builder: the samples look compatible on inspection, and that is not the same as having run them. It ran them — and designed the arm correctly. The winning arm declared no Eleven inbounds executed bodies in a child; zero The regression test pins the default across all three objects that could contradict it — settings, service settings, and the runner policy the engine actually reads — and the dataclass assertion was falsified against a different field first, so it is known to discriminate. The counting trap is the finding I would most want keptThe raw child count read 22, not 11, which looks like workers spawning workers and would have contradicted the item's "one child per inbound". It is the venv Proven on the parent process rather than reasoned about, and recorded in the ledger so nobody re-derives the phantom. That is the same instrument class that has bitten this repository all day — and note it would have been reported as a defect in the change, not as a measurement error. It caught the one place the old default was load-bearingADR 0144's rejected-alternatives rationale used "it is off by default" as one of two grounds for not relying on the sandbox alone. That ground is now gone. The rejection survives on the other — an address-space boundary does not catch PHI reaching the store's own log, or SQL built inside A flip that quietly invalidated a rejected-alternative would have left a gate standing on a premise nobody could find. That is the compensating-control-on-a-false-premise shape, avoided. Two judgement calls I agree withIt states plainly that On the ASVS method page it corrected the fact and left the verdict alone, because re-scoring is the tracking seat's act. Correct, and the same boundary it held on the vault-only rows: it declined to re-pick an anchor from a checkout that cannot read the document, on the ground that this trades a stale anchor for an unverifiable one. That is exactly right, and it is a distinction most sessions would miss. The risk, stated rather than buriedThe full suite was not run, deliberately, because this box is saturated and would have measured contention rather than the flip. This is a behaviour change on the shipped default, so that leg plus The item correctly stays open: two coupled vault-only rows remain, and it confirms the prior lane's signal that difficulty is 5 to 6 rather than 3. Labelling, and saying so. The reviewer role is retired and the required context was removed at about 23:33 UTC, so this label clears nothing and asserts nothing about independence. I commissioned this diff — I wrote the brief, so I am not independent of its scope. I read the diff, which I did not write. Not enqueued, auto-merge not armed. |
Closes the change named in BACKLOG #1278:
[sandbox].modenow defaults to"subprocess", so astock
serveruns Router and Handler bodies in a per-inbound worker child instead of the engine's ownaddress space. The isolation mode has been built and exercised since ADR 0087. Only the default was
wrong.
The item stays open for two vault-side rows a Builder cannot reach. See the last section.
First, the thing this does NOT do
It does not stop config Python executing in the engine process. The loader still runs every
*.pyin the config directory in-process, as the service account, at every
serveand every reload, ungatedby
mode. Whatmodegoverns is where a Router's or Handler's body runs once the graph is built.Module top level is out of its reach either way, and the safe-source DACL gate is still what covers it.
This is stated first in the setting docstring, in
pipeline/sandbox.py, inCONFIGURATION.mdand inDEPLOYMENT.md, because a reader must not buy a boundary that is not there.The samples were RUN, not inspected
The item warns in capitals that the shipped samples look compatible and that this is not the same as
having run them. So they were run, as a paired A/B on the same tree:
[sandbox]section at allmode=offcontrolAA./in/fhir,./dev-inbox/pdf)OB_ACME_ADT2,OB_EPIC_STREAM_MDM1_sandbox_workerchildrenSandboxErroroccurrencesSame ACKs, same deliveries, isolation on or off — so the instrument discriminates rather than merely
reporting a green. Delivery was measured against real MLLP receivers I stood up on 2601/2641/2776,
so this is the full path (ingress, sandboxed Router, sandboxed transform, outbound delivery, downstream
ACK), not just the inbound ACK.
The winning arm declared no
[sandbox]section whatsoever, which is what proves the default alonecarries it.
All 14 modules under
samples/config/loaded — 13 inbound and 12 outbound connections constructed —and 11 inbounds executed Router and Handler bodies inside a worker child. Not exercised: the DICOM
C-STORE SCP on 11112 (needs an SCU) and the
IB_RTE_RESPONSEloopback.A counting trap, so nobody re-derives a phantom 2x
The raw process count was 22, not 11, and it looked exactly like workers spawning workers — which
would have contradicted the item's finding 2. It is not. On this box
.venv\Scripts\python.exeis a redirector that re-executes with a byte-identical command line, soevery logical Python process appears twice in
Win32_Processunder the sameCommandLine. Provenon the parent: one
servelaunch produced two processes with identical command lines, andsys._base_executablediffers fromsys.executable. The real figure is one worker per inbound withtraffic, exactly as filed.
Two failures that are not mine
OB_IMMUNIZATION_BODYCREDandOB_IMMUNIZATION_REGISTRYfail to start becauseenvironments/dev.tomlcarries none of the
registry_*values. They appear identically in both arms, including themode=offcontrol, which is the proof they are pre-existing. The engine isolates them and continues.The smoke also needs
[security].handles_real_patient_data = false, orserve --env devrefuses tostart without a store encryption key.
The capability the default costs you
db_lookupandfhir_lookupare fail-closed REFUSED inside the child. They re-enter the eventloop, which a process boundary breaks. A Handler needing either must run
mode="off". That escapeis retained, supported, and tested here; it is not going away.
modeis read once at engine construction, so/config/reloaddoes not re-read it and changing itneeds a restart.
The five findings the item named
Honoured, not re-derived. ADR 0087's numbers are cited, never re-measured.
mode=offrun_sandboxedisliterally
fn(payload)with no timeout at all. Documented both ways round: a busy loop can nolonger wedge intake, and a legitimately slow Handler now dead-letters where it used to finish.
test_threat_model_doc_drift.py's label read"no Router/Handler wall cap", which the flip madebackwards; corrected.
pipe fds, and on Windows a job-object handle, per inbound with traffic. Now in
CONFIGURATION.md.accepts=predicate,
1 + 2Kfor fan-out to K. In the table beside the 0.19 ms / 6.2 ms figures, so theheadline number cannot be read without its multiplier.
fuse_thread_hopsnow carries the warning that the runner hard-disables fusion underneath itwhenever the sandbox is on. An operator reads the knob they set, not the other one.
does not."
dry_run()takes nosandboxargument and always runs in-process, so a Handler callingdb_lookup/fhir_lookuppassesmessagefoundry checkgreen and then fails closed atserve,and
wall_secondsis unenforced in the preview. Bothdryrun.pyandchecks.pynow say sooutright. Teaching the gate to spawn a worker per inbound is its own change with its own cost;
route_only/transform_onealready takesandbox=, so the seam is there when someone wants it.Note
checks.py's existing "previews what the default engine actually delivers" claim is scoped tosnapshot_on_sendand stays true — but it generalizes badly, so the limit sits right beside it.The two places the old default was load-bearing
ADR 0144's rejected-alternatives rationale — the stated reason the static lint gate exists at all —
gave two grounds for refusing to rely on the sandbox alone, and "it is opt-in/off-by-default" was one
of them. That ground is gone. The rejection survives on the other, which is sufficient alone: an
address-space boundary does not catch a Handler leaking PHI into the store's own log or building SQL
inside the sanctioned
db_lookup, because neither crosses an address space. Written so a reader cansee the gate still has a reason to exist.
docs/ASVS-ASSESSMENT-METHOD.md:115used this exact cell as its worked example of rule 5, "aworking control that ships off". That premise expired. The fact is corrected; the
partialverdictis deliberately left standing — re-scoring a cell is the tracking seat's act against the vault, not a
Builder's. The table now says plainly that no live worked example of rule 5 remains, rather than
inventing a substitute, which is the choice its own 3.7.3 row already made.
Also swept
config/settings.py,pipeline/sandbox.py(module docstring,SandboxMode.OFF),pipeline/engine.py,pipeline/wiring_runner.py,checks.py,pipeline/dryrun.py,docs/CONFIGURATION.md,docs/DEPLOYMENT.md,docs/PHI.md,docs/adr/README.md, ADR 0087 (amended, not rewritten),ADR 0144, ADR 0147,
tests/test_threat_model_doc_drift.py,tests/test_phi_logging_inventory.py.SandboxPolicy.modelost its= SandboxMode.OFF— a second default free to contradict the first. All12 construction sites already passed
mode=explicitly, so requiring it was free.tests/test_settings.pyneeded no edit: its round-trip arm already picks the opposite of whatever thecurrent default is.
Checks run vs skipped
Interpreter provenance, since a green from the primary checkout would say nothing about this change.
Run with
C:\Users\Scott\Code\MessageFoundry\.venv\Scripts\python.exefrom this worktree as cwd, andverified before trusting any result:
That path is inside this worktree, so the tests imported the code under review.
Ran, green:
test_sandbox.pytest_sandbox.py+_codec+_import_boundarytest_threat_model_doc_drift.py,test_phi_logging_inventory.py,test_settings.py,test_accepts_seam.pytest_checks.py,test_checks_gate_parity.py,test_dryrun.py,test_dryrun_snapshot_parity.pytest_doc_guards_lane.py,test_doc_ref_handle.py,test_docs_cite_no_refused_config_keys.py,test_adr_analyze.py,test_docs_security_pathways.pyruff check ./ruff format --check .mypy messagefoundry(strict)scripts/docs/backlog_status_check.pyDeliberately skipped — CI's job. The full engine suite. This is not a judgment call I made
lightly: the item's own dispatch note demands a full-suite lane, and the previous lane reverted this
exact change for want of one. The box is shared and the suite is not merely slow here, it is
unavailable — a peer measured 4 percent in 18 minutes against 28 concurrent pytest processes on 20
cores. Running it under saturation would measure the contention, not the flip. Please read the full
suite on the hosted runner before merging, along with
windows-service-smoke, which never runslocally.
Two known-spurious failures, reported by the Lander today. If
tests/test_worktree_gate_control_plane.pyreds, it is not from this branch — it is failing across unrelated PRs that do not touch it while
mainis green. If the web console leg on
windows-2025reds with 405 passed and zero failures, that is atiming gate rather than a real failure.
The doc-content half of
test_threat_model_doc_drift.pyis inert here and in CI (the vault documentis absent from every public checkout), and it emits a
ThreatModelDocUnenforcedwarning saying so. Thatis expected, not new.
What keeps the item open
Two coupled rows in the vault-only
docs/security/THREAT-MODEL.md, which no checkout can see:shipped default, not off it.
_DANGEROUS_ROW_KEYSstill anchors on"**In-process (default) or subprocess-isolated execution",which now names false vault text. The anchor is deliberately NOT re-picked. Choosing a
replacement from a checkout that cannot read the document would trade a stale anchor for an
unverifiable one and red the leg for whoever does hold the vault.
Both are marked in the test file as coupled vault-side edits, the way the
pickleanchor beside themalready is.
Re-scoring signal
The prior lane's read is confirmed: difficulty 3 was wrong, 5-6 is right. The change is one default;
the verification is a full suite this box cannot run, a sweep across roughly 14 sites in code, tests and
six documents, plus two vault documents, and five findings each needing release-note prose the row did
not budget for. Value 6 still looks right.
Zero deployments (CLAUDE.md section 0), so this cost no migration and there was nobody to notify. That
removed the vacuous cost of a staged rollout; it did not lower the bar on proving the new default works,
which is what the paired A/B above is for.
🤖 Generated with Claude Code