fix(spawn): persist every cloud-lane name the closed monitor pane must read - #286
Merged
Merged
Conversation
added 4 commits
August 21, 2026 06:09
…t read A secondmate compartment could never create a child crewmate's worker. The controller admitted the request with every binding right, then the Azure provider refused: FM_AZURE_TENANT_ID is required and must be supplied out of band. SPAWN_CLOUD_ENV_ALLOWLIST is the set written into state/<id>.cloud-env, which the monitors source in a subshell for every lifecycle call; their Herdr panes inherit nothing from the operator's shell, so a name absent from that file is unreachable in production. The allowlist carried 19 names and the deployment path reads 37. The gap was structural, not a typo. The allowlist's own regeneration recipe grepped only bin/fm-worker-lifecycle.py and bin/fm-azure-worker-provider.py, but the deployment runs in bin/fm-azure-pilot.sh, a SUBPROCESS the provider launches from run_pilot_create. Twelve names its require_cloud_environment refuses without, and the parameter file it builds, were invisible to that grep. Seven of them refuse outright; the rest, including the worker VM image id, would have drifted silently to a default. bin/fm-cloud-env-contract.py is now the one owner of that set: it derives the names from the readers, subtracts the seven the provider supplies per placement, and records secret-bearing exclusions explicitly so the allowlist stays a reviewed literal rather than a prefix glob. The new behavior test is effect-shaped. It derives the contract, exports a shape-valid value for every name, runs a real cloud spawn, then sources the persisted file in a scrubbed environment and compares value by value. It never names the reported variable, so a name added to a reader and not to the allowlist goes red on its own.
…or the derivation Review found the guard failed loud in the wrong direction. The test asserted contract subset-of persisted and never the reverse, and nothing else in the repo constrained SPAWN_CLOUD_ENV_ALLOWLIST, so adding real secret-bearing names to it with no reader stayed green, and SECRET_BEARING_EXCLUSIONS had no enforcement power at all. Worse, a bare comment in a reader pulled a name into the derived contract, and the remediation the failure printed steered the developer toward persisting it. The test now asserts EQUALITY. Its probe environment is the contract unioned with every name the allowlist currently spells, so an extra name that no reader asks for actually reaches the persisted file and fails there, on the file's own contents rather than on a source read. Both failure messages name SECRET_BEARING_EXCLUSIONS as the lever, and the extraction carries its own vacuity guard so a broken read cannot silently disarm the half of the test that looks for extras. The scan is now comment-blind: Python readers are tokenized, shell readers drop whole-line comments. That removes the steering at its source rather than arguing with it afterwards, and the derived set is unchanged at 37 names because no name was ever comment-only. It over-includes rather than under- includes by design, since a trailing shell comment still counts. provider_supplied is anchored to run_pilot_create's own body, top-level def to top-level def. The previous regex was unbounded at the start, so hoisting the env.update into a helper defined later in the same file matched the helper and returned an identical-looking 37-name set while the real deployment would have run at capacityProfile=foundation with every worker binding left unbound. That refactor now refuses by name. bin/fm-worker-lifecycle.sh joins the scanned readers. It is in the chain and names nothing today, so it was unguarded rather than safe.
…ity to the file Both round-two findings were the same species as the original defect: a comment asserting a safety the code did not provide. The provider-supplied slice is now taken by ast, from the top-level FunctionDef or AsyncFunctionDef named run_pilot_create, using end_lineno. The terminator regex it replaces had to enumerate the shapes that end a function and missed two, both of which reopened the hole it existed to close: `async def` was not in the terminator, and a run_pilot_create that is the LAST top-level def fell through to end-of-file and swallowed module-level code after it. Each returned the identical 37 names with rc=0 while the real deployment would have run at capacityProfile=foundation with every worker binding unbound. Both now refuse by name. Decorator, nested def, class-method decoy and a docstring containing "def " all still behave. The test's equality was scoped to the probe, not to the file: it intersected the file's contents with the probe before comparing, so any name written by a path other than the allowlist loop was outside the assertion entirely. The persist block really does have such paths, so they are now named in CLOUD_ENV_NON_ALLOWLIST_EXPORTS and every other name in the file is compared, whatever wrote it and whatever its prefix. A rogue export is also inert unless its variable is set, so the probe additionally carries every literal `export NAME=` the persist block can emit; the guard on that extraction caught its own first version reading one name of three. code_text no longer overstates itself. Tokenizing is exact for `#` only, and a name in a docstring or any string literal still enters the contract, which is the safe direction and now says so. Shell is no longer stripped at all: a `#` line inside a multi-line double-quoted string or an unquoted heredoc body genuinely expands, and dropping it would be an under-include, the one direction that silently loses a real name. The union is unchanged by that choice, and it retires MAY_BE_EMPTY along with the permanent quiet-exemption it granted bin/fm-worker-lifecycle.sh.
…e message Found by re-driving the red directions through the sealed entry point rather than through run-one.py. Every ContractError the derivation raises goes to stderr, and the assertion captured stdout only, so a derivation that refuses produced "the cloud-env contract could not be derived:" with nothing after the colon. The async-def hoist was red for the right reason and said nothing about what it was. stderr now goes to its own file and into the message. Not folded into the name list, because that would put refusal text where names are expected.
ruby-dlee
force-pushed
the
fix/cloud-env-allowlist-provider-contract
branch
from
August 21, 2026 10:35
85a8c36 to
99706e2
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.
The defect
A live Azure acceptance run: a secondmate compartment requested a crewmate child, the controller admitted it with every binding right, then creating the child's worker failed.
SPAWN_CLOUD_ENV_ALLOWLIST(bin/fm-spawn.sh) is the set of variables persisted intostate/<id>.cloud-env. The compartment and crewmate monitors source that file in a subshell for every lifecycle call they make, because their Herdr panes are deliberately CLOSED and inherit nothing from the operator's shell. A name missing from that file is unreachable in production on every machine, in every configuration. The compartment's own VM was created fine only because that placement ran from the operator's shell.It was not one omission. It was 21
The allowlist's own regeneration recipe was the bug's source:
The deployment does not run in the provider.
run_pilot_createshells out tobin/fm-azure-pilot.sh worker-create, a subprocess that reads its own environment. That grep could never see it. Reconciled against the readers, the persist side was missing 21 names.Refuse outright (
require_cloud_environment/os.environ[...]inmake_parameters_file):FM_AZURE_TENANT_ID,FM_AZURE_ADMIN_EMAIL,FM_AZURE_ADMIN_USERNAME,FM_AZURE_ADMIN_SSH_PUBLIC_KEY,FM_AZURE_RUNNER_OPERATOR_OBJECT_ID,FM_AZURE_KEY_VAULT_NAME,FM_AZURE_BUDGET_START_DATESilently drift to a default (
os.environ.get, all seven defined in the operator's own fleet.env or read by the lifecycle):FM_AZURE_WORKER_IMAGE_ID(the worker VM image),FM_AZURE_OPERATOR_DATA_PLANE_IP,FM_AZURE_RUNNER_VALIDATION_SKU,FM_AZURE_STEADY_STATE_BUDGET_TARGET_USD,FM_AZURE_VM_FAMILY,FM_AZURE_PROTECT_DURABLE_STATE,FM_AZURE_MUTATION_STATE_DIR,FM_AZURE_CLEANUP_TIMEOUT_SECONDS,FM_AZURE_WORKER_SLOTS,FM_AZURE_WORKER_SKUS,FM_AZURE_SECONDMATE_MAX,FM_AZURE_WORKER_DAILY_BOUND_USD,FM_AZURE_WORKER_DAILY_BOUND_OVERRIDE,FM_AZURE_WORKER_IDLE_RELEASE_SECONDSThe last four were missing even from the grep the comment claimed the line was generated by, so the line and its stated source already disagreed. Fixing only the reported name would have moved the wall one variable further in.
THREE names leave the allowlist:
FM_AZURE_CAPACITY_PROFILE,FM_AZURE_AUTHOR_CAPACITY_MODE, andFM_AZURE_WORKER_COST_ATTRIBUTION. All three appear in the provider only insiderun_pilot_create'senv.update, which hard-sets each, so an operator copy was dead weight that could only ever be a stale override.Nothing secret-bearing enters. Every added name is an identifier, a bound, a path, or a public key. The prefix genuinely does carry credentials elsewhere in the fleet:
FM_AZURE_VALIDATION_CREDENTIAL_KEY_FILE,FM_AZURE_VALIDATION_WORKTREE_KEY_FILE, andFM_AZURE_GITHUB_TOKEN_FILE(bin/fm-azure-validation.py:464). Those stay out because no scanned reader names them, which is a fact about today's readers and NOT a property the scan guarantees. Claiming otherwise would be a comment asserting a guarantee the code does not provide, which is the shape that produced this defect. The guard is bidirectional instead.The fix
bin/fm-cloud-env-contract.pyis the one owner of the set. It derives the names from the FOUR readers (bin/fm-azure-pilot.sh,bin/fm-azure-worker-provider.py,bin/fm-worker-lifecycle.py,bin/fm-worker-lifecycle.sh), subtracts the seven the provider supplies per placement, and carries an explicitSECRET_BEARING_EXCLUSIONStuple so the allowlist stays a reviewed literal and not a prefix glob. The subtraction is sliced out ofrun_pilot_createbyast, from the top-levelFunctionDef/AsyncFunctionDefwith that name, usingend_lineno. Every derivation step fails loudly rather than returning an empty set.bin/fm-spawn.shnow carries the regenerated literal and a comment that points at the contract instead of the stale grep.The test, and its proven-red evidence
test_persisted_cloud_env_matches_the_deployment_read_set_exactlyintests/fm-spawn-cloud.test.shis effect-shaped and never mentions the reported variable. It derives the contract, exports a shape-valid value for every probe name, runs a REAL cloud spawn, and then sources the file the spawn actually wrote in anenv -ienvironment scrubbed to what a Herdr pane keeps, comparing value by value.It asserts EQUALITY, not containment, and the probe environment is deliberately wider than the contract: the contract unioned with every name the allowlist currently spells, so an extra name actually reaches the file and fails on the file's own contents. A missing name is an outage. An EXTRA name is the more dangerous failure, because
SPAWN_CLOUD_ENV_ALLOWLISTis what keeps a secret-bearingFM_AZURE_*off disk, and a containment-only assertion let anyone widen it and stay green. Two vacuity guards, one per set, so neither half can silently disarm.Red with the allowlist reverted to its pre-fix value, on its own wording:
Red in the other direction too. Adding a real code read to
bin/fm-azure-pilot.shwith the fix in place:Four more, all executed, all raised by the review:
FM_AZURE_VALIDATION_CREDENTIAL_KEY_FILEandFM_AZURE_GITHUB_TOKEN_FILEadded to the allowlist with no reader naming them: red withwrites names no reader on the deployment path asks for.SECRET_BEARING_EXCLUSIONSwhile left in the allowlist: red the same way. The tuple has enforcement power now; before, it was a comment.env.updatehoisted out ofrun_pilot_createinto a helper defined later in the same file: the contract refuses by name. The old regex was unbounded at the start, matched the unrelated helper, and returned an identical-looking 37-name set, rc=0, test green, while a real deployment would have run atcapacityProfile=foundationwith all four worker bindings"unbound".FM_AZURE_CLIENT_SECRETinside a Python reader: no longer pulls it into the contract, which removes the bad steering at its source rather than arguing with it afterwards. See the scan's exact limits below.That is the whole point: a name added to one side and not the other goes red without the test having heard of it.
Green:
tests/fm-spawn-cloud.test.sh(26 assertions),tests/fm-secondmate-cloud-monitor.test.sh,tests/fm-cloud-state.test.sh,bin/fm-lint.sh(exit 0). All throughpython3 tests/run-one.py. No Azure resource was created, read, or mutated.What the scan actually does, stated exactly
Overstating this is the mistake the PR exists to catch, so:
COMMENTtokens is exact for#only. It is not a claim about mentions in general. A name inside a docstring or any other string literal is still scanned and still enters the contract, and docstrings are the natural place to document environment variables in Python, so that is a live case rather than a corner. It over-includes, which is the safe direction, andSECRET_BEARING_EXCLUSIONSis the answer.#in shell is only sometimes a comment, and a line beginning#$VARinside a multi-line double-quoted string or an unquoted heredoc body genuinely expands, so astartswith("#")filter drops a real read. Under-include is the one direction that silently loses a name and recreates the outage. No cheap shell parse tells the cases apart, so the scan no longer tries: shell comments count as reads. A raw-versus-stripped diff shows no name disappearing from any reader, and the derived set is unchanged at 37 either way.bin/fm-worker-lifecycle.shcontributes nothing to the set. All 16 of itsFM_AZURE_names sit in its header documentation and every one is covered by the other three readers. It is scanned rather than exempted, so the day it grows a real read the contract sees it, but it should not be mistaken for coverage. Dropping the shell stripping also retired theMAY_BE_EMPTYentry this reader previously needed, so it is no longer permanently quiet-exempt.Round three, four more, all executed:
env.updatemoved into anasync defhelper right afterrun_pilot_create: the terminator regex did not knowasync def, so it returned the identical 37 names, rc=0, green. Theastslicer refuses by name.run_pilot_createmade the last top-level def, with module-levelenv = os.environ.copy(); env.update({...})after it: the regex fell through toend = len(source)and swallowed it, again 37 and green. Also refuses now.def, class-method decoy, and a docstring containingdefall still derive correctly (the class-method decoy correctly adds its name to the contract as a genuine reader mention, which is the safe direction).printf 'export FM_AZURE_CLIENT_SECRET=%q\n'added tospawn_cloud_persist_convergence_artifacts, outside the allowlist loop: previously green, because the equality was intersected with the probe first. Now red. A rogue export is also inert unless its variable is set, so the probe additionally carries every literalexport NAME=the persist block can emit; the guard on that extraction caught its own first version reading one name of three, and a non-FM_AZURErogue (FM_SNEAKY_TOKEN) is caught too.All three vacuity guards fire for real: rewriting the allowlist assignment to double quotes yields
only 0 allowlist names could be read, and the literal-export guard fired during development as described above.Verified under #282's ambient seal
Rebased onto
6a41486d; #282 and #281 both landed while this was in flight. Thetests/fm-spawn-cloud.test.shoverlap with #282 rebased cleanly (differentregions), and the derived contract is unchanged at 37 names, byte-equal to the
allowlist literal.
The seal question was real and had to be established rather than assumed, because
this test's whole method is to export a value for every contract name and compare the
persisted file value by value, and
FM_AZURE_*is exactly what the seal drops. Re-runthrough
tests/run.shwith the operator'sfleet.envsourced andFM_HOME/FM_SPAWN_CLOUDexported (
run-one.pyis the reaper, not the entry point, and bypasses all of this):FM_AMBIENT_SEAL dropped=27 names, all 25FM_AZURE_*plusFM_HOMEandFM_SPAWN_CLOUD.FM_AZURE_TENANT_ID=fmcontract-FM_AZURE_TENANT_ID), not the operator's real ones.FM_AZURE_SUBSCRIPTION_IDcarries the harness fixture UUID rather than the operator's subscription, which is the seal's effect visible in the artifact.${!var+x}(set, not non-empty), so a sealed-empty value would still be written and then fail the value comparison, while an unset one fails the missing check.tests/fm-cloud-provider-seal.test.shpasses with the operator environment present.All red directions re-driven SEALED, all reproducing identically:
SECRET_BEARING_EXCLUSIONS, left in allowlistenv.updatehoisted into anasync defhelperD7 surfaced one real defect the unsealed runs had hidden: the assertion captured
the contract's stdout only, so a derivation that refuses printed
could not be derived:with nothing after the colon. Red for the right reason, silent aboutwhat it was. Fixed in
99706e2d, stderr captured separately so it reaches themessage without polluting the name list.
Sealed with the operator environment present, all green:
fm-spawn-cloud,fm-secondmate-cloud-monitor,fm-cloud-state,fm-cloud-provider-seal,fm-worker-lifecycle.bin/fm-lint.shexit 0.The drift question
Five places knew "which FM_AZURE_* a cloud lane needs", and they shared no source:
SPAWN_CLOUD_ENV_ALLOWLIST(bin/fm-spawn.sh:4523) - the persist siderequire_cloud_environment+make_parameters_file(bin/fm-azure-pilot.sh:113,556) - the deployment read sideenvironment()(bin/fm-azure-runner.py:371) - the runner laneruntime_config()(bin/fm-crosscheck-azure.py:295) - the crosscheck lanebin/fm-azure-validation.py:244- the validation cell lanePlus two non-code copies: the prose lists in
docs/azure-crosscheck.mdanddocs/azure-pilot.md, and the operator's own~/.fm-azure/fleet.env.After this PR, 1 derives from 2 mechanically and is test-enforced against the persisted file in both directions, so that pair can no longer drift. 3, 4 and 5 still learn their sets independently. Each is a read-side guard that refuses by name, so a divergence there fails loudly instead of silently, which is why this PR does not fold them in.
BUT THAT LOUDNESS IS WORTH EXACTLY WHAT IT WAS WORTH HERE, WHICH IS NOT MUCH. The pilot also refuses by name. That is precisely the refusal in this bug report, and it saved nobody: it took a live Azure acceptance run on a real subscription to surface, because no hermetic test reaches those lanes either. "Loud" in lanes 3, 4 and 5 means loud at runtime, in front of an operator, after money and a lease are already committed. It does not mean caught in CI. Treat their independence as unmeasured, not as safe.
Sibling lanes
cloud-envfile, same allowlist, same closed pane, so it carried the same 21 omissions. It has not bitten becausebin/fm-spawn-cloud-monitor.shonly ever dispatchesexecute; it never runsreconcile --apply, so it never reaches the pilot. It is fixed here by construction.bin/fm-secondmate-cloud-monitor.shsourcescloud-envat five call sites andfm-secondmate-cloud-monitor.py'sspawn_environmentpassesos.environthrough rather than keeping a second list, so the child's own spawn re-persists from what the parent's file supplied. One list, correctly, and this PR fills it.bin/fm-spawn.sh:4598-4626holds the only lines that emit into$STATE/$ID.cloud-env; the repo does contain otherexport NAME=writers, such asbin/fm-bootstrap.sh:723writingexport FM_HOME=into generated shims, which is a different channel entirely. Both lanes keep their own required-env list (4 and 5 above) and refuse by name when it is unmet, so the silent-omission shape cannot hide there. They carry the drift, not the defect. Not fixed here.Known gaps, stated not built
FM_SECONDMATE_CHILD_MAXandFM_SECONDMATE_CHILD_TOTALare the same defect class, one line away. They sit atbin/fm-worker-lifecycle.py:352-353, immediately after theFM_AZURE_SECONDMATE_MAXthis PR does add, read through the same_bounded_env_inthelper, and they gate compartment-child fan-out at:3038-3055, the exact lane this PR fixes. The contract'sFM_AZURE_regex is prefix-blind and misses them, so an operator'sFM_SECONDMATE_CHILD_MAX=8is silently ignored in the closed pane and the 4/16 defaults apply. Not an outage, because they have defaults. Not fixed here, and widening the regex would be a fix that does not fix:spawn_environment(bin/fm-secondmate-cloud-monitor.py:1376-1379) deliberately pops everyFM_SECONDMATE_*key before the child spawn, for documented reasons, so persisting them would still not reach the lifecycle process that verifies the child request. Closing it properly means deciding which side of that pop the child bounds belong on, which is its own change.bin/fm-azure-pilot.sh:116-128mandates. They are still enforced only at the pilot, after the request is admitted and the account lease is taken, so a missing name surfaces mid-lifecycle rather than at spawn. Worth building; not built here.bin/fm-worker-lifecycle.shjoins the scanned readers in this PR. It is in the chain (bin/fm-spawn.sh:4373) and has zero non-commentFM_AZURE_occurrences today, so it was unguarded rather than unsafe.