mirror of
https://github.com/mvschwarz/openrig.git
synced 2026-11-07 05:56:21 -10:00
fix(bundles): repair three review findings on the drift guard
Follow-up commit on the same branch rather than an amend — af34c2f94 is the SHA the review cites, and rewriting it would erase what was reviewed. 1. REFUSE IN BOTH DIRECTIONS. The previous revision refused only drift that dropped seats, reasoning that a spec declaring MORE is a bundle whose job is to bring the rest up. That misread what the rows are: `nodes` is the rig's PERSISTED topology, not a list of currently-running sessions. A restore that raises seats the spec never declared yields a different rig just as surely as one that drops them. Disagreement is the test, not direction; --allow-drift remains the honest way through. driftDropsLiveTopology is replaced by specDivergesFromLive, and the refusal wording no longer claims a loss. 2. VALIDATE BEFORE ASSESSING. A malformed spec has no trustworthy topology, and assessing first meant a broken file answered 409 — sending the operator to reconcile a rig when the real answer was "this does not parse". Schema validation for the selected format now runs first and keeps its existing 400; the duplicate validate calls further down the two branches are removed. 3. ENFORCEMENT FAILS CLOSED. assessSpecLiveDrift inherited a catch-all that turned any repository or query failure into null — read as "no drift", which exported the bundle. A guard that reports clean because its own query threw is worse than no guard. It now propagates and the route 500s. describeSpecLiveDrift keeps its non-throwing reporting contract, wrapping the call itself; the two contracts differ on purpose and both are pinned. Five new tests, each verified RED against af34c2f94: over-declaring spec 201→409, malformed pod-aware 409→400, malformed legacy 409→400, dropped nodes table 201→500. The pre-repair 201 on a broken database is the sharpest of them — a failed comparison produced a successful export. daemon 7392 passed / 9 failed in 4 files, identical to the pristine-source baseline (dist- and tmux-dependent integration files). cli bundle 26/26. tsc --noEmit clean on both packages. (cherry picked from commit 218c5b8112356cf29df42a291f4dafef75cf8ea0)
This commit is contained in:
v-openrig-build authored and mvschwarz committed
1 parent
374e945e7c
commit
37d2a76087
3 files changed
+165
-59
No files matched your search
@@ -252,63 +252,70 @@ function realFsOps(): FsOps {
|
||||
* Reporting only: never mutates, never blocks, never throws into the export path.
|
||||
*/
|
||||
export function describeSpecLiveDrift(rawParsed: unknown, rigRepo: RigRepository | undefined): string | null {
|
||||
const result = assessSpecLiveDrift(rawParsed, rigRepo);
|
||||
return result ? bundleExportWarning(result) : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* The same comparison as `describeSpecLiveDrift`, returned STRUCTURED rather than as a sentence.
|
||||
*
|
||||
* The warning string is for a human; a refusal has to reason about the DIRECTION of the delta, which
|
||||
* a sentence cannot be asked to do. Same null contract: no repo, a rig that is not running, or an
|
||||
* unparseable spec all yield null and the caller says nothing.
|
||||
*/
|
||||
export function assessSpecLiveDrift(rawParsed: unknown, rigRepo: RigRepository | undefined): ConformanceResult | null {
|
||||
try {
|
||||
if (!rigRepo) return null;
|
||||
const spec = rawParsed as { name?: unknown; pods?: unknown; nodes?: unknown } | null;
|
||||
const rigName = typeof spec?.name === "string" ? spec.name : "";
|
||||
if (!rigName) return null;
|
||||
// Pod-aware and legacy specs both reach this endpoint and both ship the same wrong artifact.
|
||||
// One comparator, two readers — the format only decides how ids are read, never what "drift"
|
||||
// means.
|
||||
const isPodAware = Array.isArray(spec?.pods);
|
||||
const isLegacy = !isPodAware && Array.isArray(spec?.nodes);
|
||||
if (!isPodAware && !isLegacy) return null;
|
||||
const rig = rigRepo.listRigs().find((r) => r.name === rigName);
|
||||
if (!rig) return null;
|
||||
const rows = rigRepo.db
|
||||
.prepare("SELECT logical_id FROM nodes WHERE rig_id = ?")
|
||||
.all(rig.id) as Array<{ logical_id: string | null }>;
|
||||
const liveIds = rows.map((r) => r.logical_id);
|
||||
return isPodAware
|
||||
? compareSpecToLive(topologyFromRigSpec(spec as never), topologyFromLiveLogicalIds(liveIds))
|
||||
: compareSpecToLive(topologyFromLegacyRigSpec(spec as never), topologyFromLiveNodeIds(liveIds));
|
||||
const result = assessSpecLiveDrift(rawParsed, rigRepo);
|
||||
return result ? bundleExportWarning(result) : null;
|
||||
} catch {
|
||||
// A reporting path must never be the reason an export fails.
|
||||
// REPORTING contract, deliberately different from the enforcement one below: a banner must never
|
||||
// be the reason an export fails. Enforcement cannot afford this and does not share it.
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Does this delta mean the bundle would DROP something that is running?
|
||||
* The same comparison as `describeSpecLiveDrift`, returned STRUCTURED rather than as a sentence.
|
||||
*
|
||||
* Only one direction of drift corrupts a recovery artifact. A spec that declares MORE than is live
|
||||
* is a bundle whose job is to bring the rest up — refusing that would fire the guard on exactly the
|
||||
* case it exists to serve. A spec that declares LESS silently deletes running pods and seats from
|
||||
* the only record of them, and presents as success while doing it. That is the one we refuse.
|
||||
* Null means there is genuinely nothing to compare: no repository, a spec with no `name:`, a shape
|
||||
* that is neither pod-aware nor legacy, or no rig of that name on record. Those are all honest
|
||||
* absences of a comparison.
|
||||
*
|
||||
* A REPOSITORY OR QUERY FAILURE IS NOT ONE OF THEM AND PROPAGATES. The reporting helper above may
|
||||
* swallow it, because the worst case there is a missing banner. Enforcement may not: translating a
|
||||
* failed DB read into "no drift" would let the export proceed on the strength of a comparison that
|
||||
* never happened, which is the fail-open this guard exists to remove. Callers fail closed.
|
||||
*/
|
||||
export function driftDropsLiveTopology(result: ConformanceResult | null): boolean {
|
||||
if (!result) return false;
|
||||
return result.podsMissingFromSpec.length > 0 || result.seatsMissingFromSpec.length > 0;
|
||||
export function assessSpecLiveDrift(rawParsed: unknown, rigRepo: RigRepository | undefined): ConformanceResult | null {
|
||||
if (!rigRepo) return null;
|
||||
const spec = rawParsed as { name?: unknown; pods?: unknown; nodes?: unknown } | null;
|
||||
const rigName = typeof spec?.name === "string" ? spec.name : "";
|
||||
if (!rigName) return null;
|
||||
// Pod-aware and legacy specs both reach this endpoint and both ship the same wrong artifact.
|
||||
// One comparator, two readers — the format only decides how ids are read, never what "drift"
|
||||
// means.
|
||||
const isPodAware = Array.isArray(spec?.pods);
|
||||
const isLegacy = !isPodAware && Array.isArray(spec?.nodes);
|
||||
if (!isPodAware && !isLegacy) return null;
|
||||
const rig = rigRepo.listRigs().find((r) => r.name === rigName);
|
||||
if (!rig) return null;
|
||||
const rows = rigRepo.db
|
||||
.prepare("SELECT logical_id FROM nodes WHERE rig_id = ?")
|
||||
.all(rig.id) as Array<{ logical_id: string | null }>;
|
||||
const liveIds = rows.map((r) => r.logical_id);
|
||||
return isPodAware
|
||||
? compareSpecToLive(topologyFromRigSpec(spec as never), topologyFromLiveLogicalIds(liveIds))
|
||||
: compareSpecToLive(topologyFromLegacyRigSpec(spec as never), topologyFromLiveNodeIds(liveIds));
|
||||
}
|
||||
|
||||
/** The refusal an operator can act on: what disagrees, what would be lost, and the way through. */
|
||||
/**
|
||||
* Does this spec describe a different rig than the one on record — in EITHER direction?
|
||||
*
|
||||
* An earlier revision refused only the drop direction, on the reasoning that a spec declaring MORE
|
||||
* than is live is a bundle whose job is to bring the rest up. That was wrong, and the reason is what
|
||||
* the DB rows actually are: `nodes` is the rig's PERSISTED topology, not a list of currently-running
|
||||
* sessions. A restore that brings up seats the spec never declared produces a different rig just as
|
||||
* surely as one that drops them. Direction is not the test; disagreement is. `--allow-drift` is the
|
||||
* honest way through for an operator who means it.
|
||||
*/
|
||||
export function specDivergesFromLive(result: ConformanceResult | null): boolean {
|
||||
return result ? !result.conforms : false;
|
||||
}
|
||||
|
||||
/** The refusal an operator can act on: what disagrees, and the way through. */
|
||||
export function bundleExportRefusal(result: ConformanceResult): string {
|
||||
return [
|
||||
`Refusing to bundle: this spec does not describe the rig it names — ${result.message}.`,
|
||||
`The bundle would instantiate ${result.spec.pods} pods/${result.spec.seats} seats and the running`,
|
||||
`topology above would not survive a restore from it.`,
|
||||
`The bundle would instantiate ${result.spec.pods} pods/${result.spec.seats} seats, which is not`,
|
||||
`the topology on record for this rig.`,
|
||||
`Update the spec to match, or pass --allow-drift to bundle it as-is (the divergence is then`,
|
||||
`stamped into the bundle's provenance so the artifact carries its own caveat).`,
|
||||
].join(" ");
|
||||
@@ -930,16 +937,32 @@ bundleRoutes.post("/create", async (c) => {
|
||||
const rawParsed = RigSpecCodec.parse(specYaml);
|
||||
const isPodAware = rawParsed && typeof rawParsed === "object" && Array.isArray((rawParsed as Record<string, unknown>).pods);
|
||||
|
||||
// The bundle carries the SPEC verbatim and never consults the live DB, so when a rig has been
|
||||
// expanded at runtime (no code path writes the spec back) an export ships a SMALLER rig than the
|
||||
// one running. Build B made that delta sayable; saying it on a 201 was not enough. A .rigbundle
|
||||
// VALIDATE BEFORE ASSESSING. A malformed spec has no trustworthy topology to compare, and a
|
||||
// drift 409 raised on one would blame the rig for what is actually a broken file — sending the
|
||||
// operator to reconcile a topology when the real answer is "this spec does not parse". Schema
|
||||
// errors keep their existing 400 and return before the guard ever runs.
|
||||
if (isPodAware) {
|
||||
const podValidation = RigSpecSchema.validate(rawParsed);
|
||||
if (!podValidation.valid) return c.json({ error: "Invalid pod-aware rig spec", errors: podValidation.errors }, 400);
|
||||
} else {
|
||||
const legacyValidation = LegacyRigSpecSchema.validate(rawParsed);
|
||||
if (!legacyValidation.valid) return c.json({ error: "Invalid rig spec", errors: legacyValidation.errors }, 400);
|
||||
}
|
||||
|
||||
// The bundle carries the SPEC verbatim and never consults the live DB, so when a rig's persisted
|
||||
// topology has moved (no code path writes the spec back) an export ships a DIFFERENT rig than the
|
||||
// one on record. Build B made that delta sayable; saying it on a 201 was not enough. A .rigbundle
|
||||
// is a recovery artifact, and a warning stapled to a success is read at the one moment it is
|
||||
// least affordable — so drift that would DROP running topology refuses here.
|
||||
// least affordable — so any non-conforming topology refuses here.
|
||||
//
|
||||
// This throws rather than returning null when the repository read itself fails, and that is the
|
||||
// point: an enforcement path that swallows a DB error would report "no drift" and export, which
|
||||
// is the fail-OPEN this guard exists to remove. It fails closed through the 500 below.
|
||||
const drift = assessSpecLiveDrift(rawParsed, getDeps(c).rigRepo);
|
||||
const driftWarning = drift ? bundleExportWarning(drift) : null;
|
||||
if (driftWarning) console.warn(`[bundle-create] ${driftWarning}`);
|
||||
|
||||
if (driftDropsLiveTopology(drift) && !allowDrift) {
|
||||
if (specDivergesFromLive(drift) && !allowDrift) {
|
||||
return c.json({ error: bundleExportRefusal(drift!) }, 409);
|
||||
}
|
||||
|
||||
@@ -956,9 +979,7 @@ bundleRoutes.post("/create", async (c) => {
|
||||
|
||||
if (isPodAware) {
|
||||
// Pod-aware bundle creation
|
||||
const podValidation = RigSpecSchema.validate(rawParsed);
|
||||
if (!podValidation.valid) return c.json({ error: "Invalid pod-aware rig spec", errors: podValidation.errors }, 400);
|
||||
|
||||
// Validated above, before the drift guard ran.
|
||||
const effectiveRigRoot = rigRoot ? nodePath.resolve(rigRoot) : nodePath.dirname(nodePath.resolve(specPath));
|
||||
const tmpStaging = fs.mkdtempSync(nodePath.join(os.tmpdir(), "pod-bundle-create-"));
|
||||
try {
|
||||
@@ -989,8 +1010,7 @@ bundleRoutes.post("/create", async (c) => {
|
||||
}
|
||||
|
||||
// Legacy bundle creation
|
||||
const validation = LegacyRigSpecSchema.validate(rawParsed);
|
||||
if (!validation.valid) return c.json({ error: "Invalid rig spec", errors: validation.errors }, 400);
|
||||
// Validated above, before the drift guard ran.
|
||||
const spec = LegacyRigSpecSchema.normalize(rawParsed);
|
||||
|
||||
const specDir = nodePath.dirname(nodePath.resolve(specPath));
|
||||
|
||||
@@ -214,19 +214,94 @@ describe("bundle create — refuses a spec that disagrees with the live rig", ()
|
||||
expect(fs.existsSync(path.join(tmpDir, "legacy-conforming.rigbundle"))).toBe(true);
|
||||
});
|
||||
|
||||
// PRESERVE 3 — the drift that runs the OTHER way (spec declares more than is running) is a
|
||||
// legitimate export: a bundle whose job is to bring the missing pods up. Refusing it would make
|
||||
// the guard fire on exactly the recovery case it exists to protect.
|
||||
it("does not refuse when the spec declares MORE than is currently running", async () => {
|
||||
// DIRECTION IS NOT THE TEST — DISAGREEMENT IS. An earlier revision let this case through, reasoning
|
||||
// that a spec declaring MORE than is live is a bundle whose job is to bring the rest up. That read
|
||||
// the `nodes` rows as "currently-running sessions"; they are the rig's PERSISTED topology. A
|
||||
// restore that raises seats the spec never declared yields a different rig just as surely as one
|
||||
// that drops them, so it refuses too, and --allow-drift is the way through.
|
||||
it("refuses when the spec declares MORE than the rig's persisted topology", async () => {
|
||||
seedLiveRig(RIG_NAME, ["dev.impl"]);
|
||||
const specPath = writeSpec(RIG_NAME, [
|
||||
{ id: "dev", members: ["impl"] },
|
||||
{ id: "orch", members: ["lead"] },
|
||||
]);
|
||||
|
||||
const res = await create(specPath, "recovery");
|
||||
const res = await create(specPath, "extra");
|
||||
|
||||
expect(res.status).toBe(409);
|
||||
const body = await res.json() as { error: string };
|
||||
expect(body.error).toContain("orch.lead");
|
||||
expect(fs.existsSync(path.join(tmpDir, "extra.rigbundle"))).toBe(false);
|
||||
});
|
||||
|
||||
it("still exports the over-declaring spec when the operator passes allowDrift", async () => {
|
||||
seedLiveRig(RIG_NAME, ["dev.impl"]);
|
||||
const specPath = writeSpec(RIG_NAME, [
|
||||
{ id: "dev", members: ["impl"] },
|
||||
{ id: "orch", members: ["lead"] },
|
||||
]);
|
||||
|
||||
const res = await create(specPath, "extra-allowed", { allowDrift: true });
|
||||
|
||||
expect(res.status).toBe(201);
|
||||
expect(fs.existsSync(path.join(tmpDir, "recovery.rigbundle"))).toBe(true);
|
||||
expect(fs.existsSync(path.join(tmpDir, "extra-allowed.rigbundle"))).toBe(true);
|
||||
});
|
||||
|
||||
// A MALFORMED SPEC IS A BROKEN FILE, NOT A DRIFTED RIG. Both fixtures below are ALSO in drift, so
|
||||
// an implementation that assesses before validating answers 409 and sends the operator off to
|
||||
// reconcile a topology when the real answer is "this spec does not parse".
|
||||
it("returns the schema 400 — not a drift 409 — for a malformed pod-aware spec", async () => {
|
||||
seedLiveRig(RIG_NAME, ["dev.impl", "dev.qa", "orch.lead"]);
|
||||
const specPath = path.join(tmpDir, "malformed-pod.yaml");
|
||||
fs.writeFileSync(specPath, [
|
||||
'version: "0.2"',
|
||||
`name: ${RIG_NAME}`,
|
||||
"pods:",
|
||||
" - id: dev",
|
||||
" label: Dev",
|
||||
" members:",
|
||||
" - id: impl", // no agent_ref, no runtime, no profile
|
||||
" edges: []",
|
||||
"edges: []",
|
||||
].join("\n"));
|
||||
|
||||
const res = await create(specPath, "malformed-pod");
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
const body = await res.json() as { error: string };
|
||||
expect(body.error).toContain("Invalid pod-aware rig spec");
|
||||
});
|
||||
|
||||
it("returns the schema 400 — not a drift 409 — for a malformed legacy spec", async () => {
|
||||
seedLiveRig(RIG_NAME, ["dev", "qa", "lead"]);
|
||||
const specPath = path.join(tmpDir, "malformed-legacy.yaml");
|
||||
fs.writeFileSync(specPath, [
|
||||
"schema_version: 1",
|
||||
`name: ${RIG_NAME}`,
|
||||
'version: "1.0"',
|
||||
"nodes:",
|
||||
" - id: dev", // no runtime
|
||||
"edges: []",
|
||||
].join("\n"));
|
||||
|
||||
const res = await create(specPath, "malformed-legacy");
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
const body = await res.json() as { error: string };
|
||||
expect(body.error).toContain("Invalid rig spec");
|
||||
});
|
||||
|
||||
// FAIL CLOSED. If the comparison itself cannot be performed, the one answer that must never be
|
||||
// produced is "no drift, exported". A guard that reports clean because its own query threw is
|
||||
// worse than no guard: it is a green nobody can trust, on a recovery artifact.
|
||||
it("fails closed when the topology query itself fails — never a silent clean export", async () => {
|
||||
seedLiveRig(RIG_NAME, ["dev.impl", "dev.qa"]);
|
||||
const specPath = writeSpec(RIG_NAME, [{ id: "dev", members: ["impl"] }]);
|
||||
db.prepare("DROP TABLE nodes").run();
|
||||
|
||||
const res = await create(specPath, "db-broken");
|
||||
|
||||
expect(res.status).toBe(500);
|
||||
expect(fs.existsSync(path.join(tmpDir, "db-broken.rigbundle"))).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -84,6 +84,17 @@ describe("bundle export — spec-vs-live drift warning", () => {
|
||||
expect(describeSpecLiveDrift(SPEC, undefined)).toBeNull();
|
||||
});
|
||||
|
||||
// The two helpers deliberately DIVERGE on repository failure, and this pins the reporting half.
|
||||
// `describeSpecLiveDrift` swallows it — the worst case for a banner is a missing banner. The
|
||||
// enforcement half (`assessSpecLiveDrift`) propagates instead, so the create route fails closed
|
||||
// rather than exporting on a comparison that never ran; that side is pinned at the route.
|
||||
it("SILENT rather than throwing when the topology query itself fails", () => {
|
||||
seedRig("drift-rig", ["orch.lead", "dev.driver"]);
|
||||
db.prepare("DROP TABLE nodes").run();
|
||||
expect(() => describeSpecLiveDrift(SPEC, repo)).not.toThrow();
|
||||
expect(describeSpecLiveDrift(SPEC, repo)).toBeNull();
|
||||
});
|
||||
|
||||
it("never throws into the export path, even on a hostile spec value", () => {
|
||||
seedRig("drift-rig", ["orch.lead"]);
|
||||
for (const bad of [null, undefined, 42, "a string", { name: "drift-rig", pods: "not-an-array" }]) {
|
||||
|
||||
Reference in new issue
Block a user