Skip to content

live smokes call down on an unenforced sandbox assumption: a broken sandbox tears down the operator's mesh and still reports a pass #884

Description

@davidfarah2003

Nine live smokes call a down path. All of them sandbox correctly. None of them verifies the
sandbox actually held before running the destructive verb, and that gap is the whole issue: a
sandbox that silently fails to apply turns cotal down into a teardown of whatever mesh the
ambient registry happens to point at, while the suite still reports a pass.

The current shape is good, which is what makes the gap easy to miss

The guard is real, not a naming convention. dogfood-live.smoke.ts builds

const env = { ...process.env, XDG_CONFIG_HOME: configDir, COTAL_HOME: home };
const cotalAt = (cwd: string, args: string[], timeout = 180_000) => …

and runs every CLI call with cwd: root under a temp root. orca-extension-live.smoke.ts does the
same. Env and cwd, both set. On a correctly-constructed run there is no path from these suites to
an operator's mesh.

The problem is that nothing checks. If the temp root is missing, if a helper is called before the
sandbox is built, if a refactor drops env from one spawnSync among many, or if a future wrapper
overwrites COTAL_HOME, then cotal down resolves the real registry. And down's own behaviour
at that point is documented and total, in implementations/cli/src/commands/down.ts:95:

bare cotal down always stops this folder's stack

So the failure is: destructive, silent, and green. The suite tears the operator's stack down, its
own assertions still pass because the CLI did what it was asked, and nothing in the output
distinguishes "torn down my sandbox" from "torn down your fleet".

Affected files

backup-faults-live, backup-conservation-live, backup-usermode-live, backup-restore-live,
ext-live, dogfood-live, orca-extension-live, up-tls-routes-live, up-stack-live.

Grepping each for anything resembling a pre-down mesh assertion: four have zero,
three have one incidental match, one has two, and only up-tls-routes-live has a meaningful
count. So this is not one careless file, it is an unstated assumption nine files share.

Suggested fix: one shared helper, not nine edits

A single guard that resolves the mesh the CLI would act on and refuses unless it is the sandbox's
own, called before any down in a live smoke. Something with the shape of:

assertSandboxMesh(env, root);   // throws naming the resolved root vs the expected one

Nine separate edits would leave the tenth suite, written next month, unguarded. The point is that
the assumption becomes enforced in one place rather than remembered in nine.

The same reasoning applies to any harness outside this repo that drives the installed CLI and calls
down on a teardown path: the guard belongs with the destructive call, not with the convention
that is supposed to keep it away.

Related: the one-word blast-radius difference

Worth considering in the same change, because it is the same root: a destructive verb whose scope
is not confirmed before it acts. cotal down and cotal down manager differ by one word and by
their entire blast radius: the first stops broker, manager, delivery daemon and web dashboard, the
second stops the manager alone. --dry-run is the only preview and it is opt-in.

down.ts already knows the difference and explains it clearly in an error message for a different
mistake (--space with no components). The bare form gets no such narration.

Measured, since it prompted this: a scoped cotal down manager on a live fleet is a cheap re-seat;
broker, JetStream, delivery daemon and channel registry all survive. The bare form on the same
folder is a full outage. Nothing at the call site marks that difference, in the smokes or at an
operator's prompt.

Why this is worth fixing rather than noting

A destructive command that is correctly scoped by an unenforced assumption is indistinguishable, in
every log it produces, from one that is scoped by an enforced one, right up until the assumption
does not hold. The suites are green either way.

Correction to the file list above: the count was wrong, and the way it was wrong is the issue itself

The nine files named above were found by matching *live*.smoke.ts on FILENAME. That is a naming
convention, and it does not classify this set correctly. Neither does the obvious replacement.

Three separate attempts to enumerate the affected suites produced three different answers, and each
one looked complete to the person who made it:

  • Filter by filename containing "live" and you miss two suites that invoke the verb:
    implementations/auth/smoke/down-manifest-usermode.smoke.ts ("down" at 186, 240, 255) and
    implementations/auth/smoke/user-auth-launch.smoke.ts ("down" at 313, 339).
    smoke:down-manifest-usermode:live is in the check gate; smoke:user-auth-launch:live is not.
    Line 255 and line 339 are each a bare cotal down on a CLEANUP path, which is the
    worst position in the set: cleanup runs when the suite is already failing, and a suite that is
    already failing is exactly the case where the sandbox is least likely to have held.
  • Filter by script name containing :live or -live instead, and you miss
    bin/smoke/up-tls-routes-live.smoke.ts, whose script is plain smoke:up-tls-routes with no live
    suffix at all. That file is in the original list of nine, so this rule drops a suite the issue
    already knew about.
  • Neither filter can see implementations/auth/smoke/_ps-arm2.smoke.ts, which invokes the verb
    and is reached by no package.json script at all (another suite invokes it).

Measured on the tree: 41 scripts carry a live-ish name, 14 of those point at a file that invokes the
quoted down verb, and 23 smoke files invoke it in total. The set can be re-derived rather than
trusted:

node -e 'const fs=require("fs"),cp=require("child_process");
const p=require("./package.json").scripts;
const all=cp.execSync("grep -rl \"[\\\"'\"'\"']down[\\\"'\"'\"']\" --include=*.smoke.ts . --exclude-dir=node_modules",{encoding:"utf8"}).trim().split("\n").map(s=>s.replace(/^\.\//,""));
const byFile={};
for (const [k,v] of Object.entries(p)) for (const f of (v.match(/[\w./-]+\.ts/g)||[])) (byFile[f] ||= []).push(k);
for (const f of all.sort()) console.log(f.padEnd(62), (byFile[f]||["(NO SCRIPT)"]).join(","));'

Why this belongs in the issue and not only in the fix

A reader who implements the nine named files produces a change that looks complete, passes review,
and leaves two suites tearing down on a cleanup path. An issue that undercounts is a trap for
whoever picks it up next.

There is a sharper point. This issue is about an assumption that was remembered rather than
enforced, and its own file list was produced by an unenforced assumption: that a live suite is
spelled "live". Every classifier reached for so far has been a naming convention, and the defect ate
its own investigation twice before this was noticed.

So the fix should not hand-maintain a list of eleven, or fourteen, or twenty-three. A list is the
same defect one generation later: suite number twenty-four gets written next month by someone who
never read this issue, and it is unguarded on the day it lands. The enforcement belongs with the
destructive call, so that coverage is a property of the code path rather than of anyone's
enumeration, and an unguarded down becomes something you cannot write by accident rather than
something a reviewer is expected to notice. That is the same conclusion the section above already
reaches ("the guard belongs with the destructive call, not with the convention that is supposed to
keep it away"), arrived at from the other direction.

Two constraints the fix will hit immediately

Both are places where the convenient repair is a silent degradation, which this repo does not allow:

  • user-auth-launch.smoke.ts and down-manifest-usermode.smoke.ts set no XDG_CONFIG_HOME at all.
    If that variable is load-bearing for what down resolves, those suites are under-sandboxed today,
    and the correct outcome is that they refuse until fixed. An optional field, or skipping the check
    when the value is absent, is fail-open: it turns "I cannot establish the sandbox" into a pass.
  • user-auth-launch.smoke.ts:41, down-manifest-usermode.smoke.ts:40 and
    backup-usermode-live.smoke.ts:34 mutate process.env.COTAL_HOME globally rather than passing a
    per-spawn env, so a correctly sandboxed call can carry no env key at all. A guard that reads
    options.env sees nothing there, and "absent therefore fine" is the same fail-open.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:cibugSomething isn't workingseverity:criticalConfirmed critical-impact defect or security issue.triage:confirmedReported defect reproduces, or requested non-bug gap is independently verified.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions