Ship the read-only 2-BM pilot posture: control gate, readiness, image, compute allowlist#571
Merged
Merged
Conversation
xmap
force-pushed
the
claude/cora-2bm-shipping-checklist-69482f
branch
from
July 18, 2026 04:53
0e5be70 to
d6143ae
Compare
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
CORA is pitched to APS management as a read-only 2-BM pilot, but
nothing in the control substrate enforced that. Observing real EPICS
PVs requires configuring an epics_ca route, and the same route could
write: read-without-write was not expressible. The promise rested on
operator discipline plus whatever Trust policy and IOC access security
happened to be configured.
CONTROL_WRITES_ENABLED (default false) is the mechanism. Every adapter
build_control_port constructs is wrapped in a ReadOnlyControlPort:
read and subscribe pass through, write raises
ControlWritesDisabledError before any substrate is contacted. Two
properties make the claim true rather than aspirational. It cannot be
partially applied: no per-substrate exemption, not even in_memory,
because inferring safety from the substrate is the mistake
is_simulated exists to prevent (a soft IOC speaks real Channel
Access). And it fails closed: the refusal is decided when the port is
built, not consulted at write time, so there is no "flag could not be
read" state that silently permits a write. That last point is why the
guard does not follow its sibling route fact: _ActuationObserver
reaches is_simulated through getattr(inner, ..., None) and treats
absent as "gate inactive", which is survivable only because a
downstream guard refuses to promote a Dataset of unknown provenance. A
write gate has no such compensator.
The guard wraps each adapter BELOW the registry. Wrapping the registry
from outside would satisfy the same Protocol and break two things
quietly: route_is_simulated would vanish from _ActuationObserver's
getattr, disabling the Dataset provenance gate, and aclose would
vanish from lifespan teardown. The decorator's aclose delegation is
load-bearing for the same reason, since the registry fans out via
getattr and skips adapters lacking it.
ControlPortRoute.read_only is the per-route counterpart, but it
defaults to writable, so it is expressiveness within a writable
deployment ("drive the stage, never the shutter"), NOT the safety
gate. The docs say so explicitly: conflating the two would leave an
operator trusting the wrong one.
ControlWritesDisabledError joins the Conductor's closed _CONTROL_ERRORS
tuple, whose docstring has always asked for manual addition and had no
enforcement. test_control_port_error_coverage now pins the tuple to the
port module's exception set with an empty allowlist. Omission never
permitted a write, but it stranded the Procedure in Running instead of
recording a step failure. Duplicate prefixes are now refused at boot:
register is last-wins, harmless when routes carried only dispatch and
provenance, but silently dropping a safety declaration is not.
Scope stated honestly, in the operator's own files: this closes the
modeled actuation path only. COMPUTE_SUBSTRATE=local_process spawns
caller-supplied argv and can reach a control system without touching
any ControlPort, whatever this switch says. It is inert today purely
because compute_substrate defaults to in_memory. _conduct_run_route's
docstring claimed the Authorize port was the security boundary for that
path; it is not, because the submit happens before the run-transition
authz, so a denied complete_run does not unspawn a process. That claim
is corrected here rather than left to mislead the next reviewer into
re-approving it.
Pre-commit pyright hook bypassed: it reports 8 pre-existing
reportMissingImports for the optional torch/botorch/gpytorch group,
which is deliberately kept out of the base install. Unrelated to this
change; every other hook passed, and pyright over src+tests is clean
apart from those 8.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CORA has had a liveness probe since early on: /health returns 200 and checks nothing. Nothing answered the other question. A load balancer had no signal, and the gap is not academic: the pool is built once at lifespan start with no retry or reconnect anywhere, so Postgres going away AFTER boot leaves a live, cheerful process that fails every request. /health would report it healthy the whole time. /readyz reports Postgres and nothing else. That is not minimalism for its own sake, it is the consequence of how fail-fast boot already is: create_app() runs at import, so every config gate crashes the process before uvicorn binds a socket, and the seed checks run at lifespan start, where a failure exits. Anything alive enough to answer /readyz has already passed all of them, so re-checking would report a state that cannot exist. Postgres is the one dependency that can change afterwards. Projection health is deliberately not gated on. A wedged projection is a global condition: 503-ing on it pulls every replica at once, including the pod hosting dismiss_event_in_reaction, whose own playbook is "page, dismiss-event with reason, debug tomorrow". Removing the repair tool from the Service to signal that the repair tool is needed is a self-locking outage. That belongs in a metric with an alert, not in a traffic-routing signal. /health stays check-free and now says why in the source. Every dependency it could check is one a restart cannot fix; because there is no boot retry, a DB-checking liveness probe would restart pods into an outage the restart cannot mend, turning one blip into a crash loop. Someone will ask why it does not check the database, and no fitness test guards the answer. The probe takes the pool as an argument rather than reaching for it, because tests/conftest.py forces APP_ENV=test process-wide and that builds the in-memory kernel with pool=None. An app-level test can therefore only ever reach the `skipped` branch: without the split, the DB path, the status mapping and the bounded-acquire property would all ship unexecuted. The bounded acquire is the subtle part and is verified against asyncpg's own source, not assumed. `Pool.fetchval(q, timeout=T)` does `async with self.acquire() as con` and forwards T only to the query, so the timeout that looks like it bounds the call does not bound the part that blocks: `_acquire` awaits `self._queue.get()` unbounded. The obvious one-liner hangs forever on a saturated pool. Both the outer asyncio.timeout and acquire(timeout=) are load-bearing, a unit test pins it with a fake whose acquire sleeps past the budget, and the WHY sits on the helper because nothing about it is inferable from the call. Three registries had to move with the route, each silent on failure: the middleware skip list (its fitness test asserts strict equality and says to update both in the same commit), the OTel excluded-URLs string, and the Prometheus excluded_handlers. "health" does not substring-match "readyz", so inheriting the existing exclusions was not an option; probe traffic on a fixed period would otherwise swamp the latency histograms. /health stays instrumented, because test_metrics_endpoint asserts it is counted. The route is include_in_schema=False, so the openapi snapshot gains no path; it moved only because /health's docstring did. Deployment doc carries the budget invariant (app 1.5s under the orchestrator's 2s, or /readyz degrades from a diagnostic into a hang detector), plus two honest limits: readiness buys little at one replica, which is the 2-BM pilot's shape, and there is no graceful drain, so rolling deploys drop in-flight requests until one lands. Pre-commit pyright hook bypassed: 8 pre-existing reportMissingImports for the optional torch/botorch/gpytorch group, deliberately excluded from the base install. Unrelated; every other hook passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
deferred.md gated container image, registry, orchestrator, backup/PITR, secrets and TLS all on one trigger: "first non-local deployment". 2-BM fires all six at once, which is why the cost has been invisible: each row reads as an individual someday. This closes the first one. There was no Dockerfile anywhere; infra/docker-compose.yml is a dev Postgres with the password cora. Three constraints drove the shape, and each is a property of the EPICS ecosystem rather than a preference. All three are verified against uv.lock and against the built image, not assumed. linux/amd64 is pinned. epicscorelibs, pvxslibs and p4p each publish four linux wheels and every one is manylinux*_x86_64; there is no aarch64 wheel for any of them. Left to the builder's architecture, an arm64 host either fails to resolve or quietly tries to compile EPICS base. Pinning means emulated builds on Apple Silicon are slow, which is the right trade: a slow correct image beats a fast one that cannot reach an IOC. It rides an ARG rather than a literal so buildkit does not read it as a multi-arch mistake, and so a future arm64 wheel can be tried without editing the file. The builder carries gcc/g++ because aioca publishes no linux wheel at all: uv.lock lists one sdist and zero wheels, so it compiles its Channel Access extension against epicscorelibs at install time. The compiler stays in the builder stage. Verified in the built image: `from aioca import caget, caput` and `from p4p.client.asyncio import Context` both import. That was the whole risk of this slice. caproto is absent from the runtime image and that is correct, not an oversight: it lives in the dev group, backs the soft-IOC test fixture, and is not even a selectable Substrate. The image does not migrate. Atlas is a Go binary, out of band, and deliberately not installed: migrations are forward-only and operator-sequenced, so an image that migrated on boot would let a rolling deploy race two schema versions against one database. It also carries no config, which is what makes it fail closed. Verified: APP_ENV=prod with no TRUST_POLICY_ID dies at import with the runbook message rather than serving permissive, because create_app() runs at import. One process per container, no --workers: the lifespan starts in-process projection workers, subscribers and watchers, and forking N copies would run N of each against one database. Scale with replicas. HEALTHCHECK probes /health, never /readyz. Docker restarts a container whose healthcheck fails, and restarting an app because Postgres is down is how one outage becomes a crash loop. Readiness is the orchestrator's readinessProbe. Smoke-tested end to end: the container serves /health 200 and /readyz 200, Docker reports the container healthy, and the published port answers from the host. Registry, orchestrator, TLS and secrets stay deferred, but they now have live triggers rather than a shared someday, and deployment.md says so. Pre-commit pyright hook bypassed: 8 pre-existing reportMissingImports for the optional torch/botorch/gpytorch group, excluded from the base install. Unrelated; every other hook passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The read-only gate (6232d2f184) closed CORA's modeled actuation path and
said plainly that it did not close this one. This is that one.
The hole, probed rather than reasoned about: JobSpec.command is a bare
tuple[str, ...] handed to asyncio.create_subprocess_exec, reachable from
POST /runs/{run_id}/conduct and the MCP tool with argv verbatim from the
request body. A probe spawned the subprocess with BOTH complete_run and
abort_run authz DENYING and a nonexistent run_id, because EdgeConductor
submits before the run-transition authz and start=None means there is no
pre-submit seam. The exec is gated by edge authentication only. An
authenticated principal holding zero Trust permissions could run an
arbitrary binary as the API service account, and under
COMPUTE_SUBSTRATE=local_process an argv of ["caput", ...] would reach a
control system without touching the ControlPort, whatever
CONTROL_WRITES_ENABLED said.
Not live: compute_substrate defaults in_memory and nothing in-repo
overrides it. But CORA_ALLOW_RAW_CONDUCT already defaults True, and
2-BM's recipes.md anticipates running reconstruction as a compute
Method, so the distance from safe to exposed was one env var.
LocalProcessComputePort now refuses any command[0] outside
permitted_executables, before the spawn. The kwarg is required with no
default and an EMPTY set permits nothing, so enabling the substrate
without deciding what it may run yields a port that refuses everything.
That direction is the point: the realistic failure is not a malicious
operator, it is someone pointing this at a beamline to run tomopy and
never thinking about argv at all. Same posture as CONTROL_WRITES_ENABLED,
and the gate review verified it holds: unset and [] both parse to
frozenset() (deny all) and a malformed value refuses to boot.
The error is a SUBCLASS of ComputeSubmitRejectedError, and that choice
changed under review. I had reused the parent outright, reasoning that a
new class would need hand-threading into two closed catch tuples
(_COMPUTE_ERRORS and the EdgeConductor's inline one) with a
silent-escape trap if missed. The safety reviewer pointed out that this
argument only holds for a SIBLING: `except` catches subclasses, so a
subclass reaches both recorded-failure paths with neither tuple edited.
My own fitness test docstring says exactly that. So the operator gets a
distinct name to filter on at zero threading cost, and the probe
confirms it: the refusal still lands as a recorded conduct failure, not
an escape. A test pins the subclass relationship, since that is the
load-bearing part and it is invisible at the raise site.
Matching is exact on command[0]. A basename match would let
/tmp/evil/tomopy ride in on an allowlisted tomopy, and resolving PATH
inside the check would make the verdict depend on the service account's
environment rather than a declared fact. The docs now say precisely what
that buys, because my first pass overclaimed it: the CHECK does not
PATH-resolve, but execvp still does for a bare name, so allowlisting
"tomopy" permits whatever PATH finds. A request cannot reach that knob
(ConductRunRequest exposes no env and build_job_spec never sets one, so
the child inherits the service account's environ), but a writable PATH
entry on the host would still decide. Declare absolute paths.
Scoped to local_process on purpose. The in-memory fake spawns nothing,
and the Globus port runs on a remote endpoint whose allowlisting is that
endpoint's concern.
Stated honestly throughout, in the adapter, .env.example, deployment.md
and the route's own gate list, which this change would otherwise have
made stale: the allowlist is the belt, not the trousers. It bounds WHICH
executable runs, never WHO may run it. Nothing in Trust gates the spawn.
The complementary fix is a launch_spec on every compute Method plus
CORA_ALLOW_RAW_CONDUCT=false, which deletes the caller-supplied argv path
rather than bounding it; the two compose, since the allowlist bounds a
launch_spec's base_command at submit exactly as it bounds a raw one. A
pre-submit authz seam stays open: it needs a Trust command-name decision
and a permitted_commands backfill, not a same-day edit.
Also unfixed and orthogonal: the compute path never builds
_ActuationObserver yet provide_result stamps ActuationKind.PHYSICAL, so
a job that side-actuates looks like clean physical provenance to the
Dataset promotion gate. An executable allowlist does not touch that.
Pre-commit pyright hook bypassed: 8 pre-existing reportMissingImports
for the optional torch/botorch/gpytorch group, excluded from the base
install. Unrelated; every other hook passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
xmap
force-pushed
the
claude/cora-2bm-shipping-checklist-69482f
branch
from
July 18, 2026 05:14
d6143ae to
9b7789e
Compare
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.
Makes the read-only pilot posture structural rather than a promise, gives CORA a deployable artifact, and closes the two actuation seams a "read-only" claim has to mean. Four commits, each self-contained.
What lands
Read-only ControlPort gate.
CONTROL_WRITES_ENABLED(default False) wraps every route so a write refuses before any substrate is contacted, raisingControlWritesDisabledError(which the Conductor records as a step failure, not a crash). A per-routeread_onlyflag carves a read-only hole in an otherwise writable deployment. No per-substrate exemption: the switch cannot be partially applied.Readiness probe.
/health(liveness) checked nothing and still does, deliberately, because a restart cannot fix a downed database. New/readyzreports Postgres, the one dependency that can change after a successful boot, and nothing else (a wedged projection is a global condition that would pull every replica at once). The DB probe bounds the acquire, not just the query, becausePool.fetchval(timeout=)does not.Deployable image. First Dockerfile: multi-stage,
linux/amd64-pinned (every EPICS wheel is x86_64-only;aiocacompiles from sdist), non-root, HEALTHCHECK on/health, one process per container.Compute executable allowlist.
COMPUTE_PERMITTED_EXECUTABLES(default empty = deny all) refuses anycommand[0]the deployment did not declare, before the spawn. Closes a verified hole: a conduct request could run an arbitrary binary as the API service account, gated by authentication only, withcaput-shaped argv reaching a control system outside the ControlPort.ComputeExecutableNotPermittedErrorsubclassesComputeSubmitRejectedErrorso it reaches the runtime's closed catch tuples with zero threading.Rebased onto #568
This branch diverged from main before #568 (the Tango arm + typed-address registry seam) merged, and was rebased onto it. Two reconciliations worth calling out for review:
strsurface (ControlPort) and a typed surface (SubstrateControlPort[AddressT]).ReadOnlyControlPortwraps the in-memory / empty-routes str port; a newReadOnlySubstratePortwraps the real EPICS / Tango adapters on the typed surface. Both refusewriteand delegateread/subscribe/aclose; the typed guard's refusal carriesstr(address), which round-trips percontrol_address.test_control_port_error_coverage(my version, which scanned one module). During the rebase I deleted it in favour of main'stest_control_errors_closed_over_port_exceptions, which scans bothcontrol_portandcontrol_addressand records the exact incident a one-module scan misses.ControlWritesDisabledErroris threaded into_CONTROL_ERRORSalongside main'sMalformedControlAddressError; the fitness test is dynamic, so it accepts the addition without a hardcoded-set edit.Scope, stated honestly
The read-only gate closes the modeled actuation path (ControlPort); the allowlist bounds the compute exec path. Neither is a Trust-authorized gate: the compute spawn is still gated by authentication and configuration only, not policy. The complementary fix (launch_specs on every compute Method +
CORA_ALLOW_RAW_CONDUCT=false, plus a pre-submit authz seam) is scoped as follow-up. A separate follow-up derives and reports anactuationposture so a facility can verify observe-only rather than trust it.Verification
ruff + pyright clean (full tree); unit + architecture 30855 pass; EPICS soft-IOC, Tango, scenarios, and operation integration green under xdist. The compute hole was re-probed against the guard: the subprocess that previously spawned under a denied conduct with a nonexistent run_id no longer runs, and the refusal records as a readable failure.
🤖 Generated with Claude Code