Nate Swenson

One state string cannot carry two independent facts

Shipflow · No. 111 · 13 min read ·v0.6.0

Shipped

release-cut no longer guesses when it cannot tell which version it is being asked to release. The status reader now reports “dev is ahead of main” as its own fact rather than folding it into a single state string, a new pure resolveReleaseTarget is the only place the release target is decided, and cut calls it before any network request and refuses outright when the answer is ambiguous, naming both candidate versions in the refusal.

The general shape is worth stealing for anything that takes an irreversible action based on state it read a moment earlier: keep independent facts independent, resolve the target once, and make your escape hatch a confirmation rather than a bypass.

The state that could only report one thing

I found this the boring way, which is the only reason I am writing about it instead of apologising for it. I was about to cut a release, opened the function that does the cutting because I wanted to check something unrelated, and realised while reading it that the version it was going to tag was not the version I thought I was releasing. Main was at 0.2.1. Dev was at 0.3.0. The tool would have dispatched a release for 0.2.1, waited for that tag, found it, and reported success.

Nothing about that outcome would have looked like a failure.

The reader collapsed three inputs into one answer:

// Reads the three version facts and reports a state. The bug lives here.
export function cmpSemver(a, b) {
  const pa = String(a).split('.').map(Number);
  const pb = String(b).split('.').map(Number);
  for (let i = 0; i < 3; i++) {
    if ((pa[i] ?? 0) !== (pb[i] ?? 0)) return (pa[i] ?? 0) - (pb[i] ?? 0);
  }
  return 0;
}

// `facts` is whatever you can read off the remote: the newest tag for this
// component, and the version declared on each long-lived branch.
export function readStatus({ name, lastTag, versionOnMain, versionOnDev }) {
  let state;
  if (!lastTag) state = 'untagged-bump-on-main';       // never released
  else if (cmpSemver(versionOnMain, lastTag) > 0) state = 'untagged-bump-on-main';
  else if (cmpSemver(versionOnDev, versionOnMain) > 0) state = 'bump-on-dev-unpromoted';
  else state = 'clean';

  return { name, state, lastTag, versionOnMain, versionOnDev };
}

Look at what an if/else if chain does to two facts that are both true. “Main carries an untagged bump” and “dev already carries something higher” are not alternatives. They happen together all the time. But a chain has to pick one branch, so the first condition wins and the second fact never reaches the caller. The returned object still contains versionOnDev, and the fast path downstream never read it, because it was written to trust state.

This is the well-known advice about making illegal states unrepresentable running in the other direction. Squeezing your model down until only valid states fit is good; squeezing it past that point makes valid states unrepresentable, and the ones that vanish are the awkward combinations you did not picture while writing the enum.

Report the second fact beside the state, not inside it

The fix is small and the placement is the entire trick: compute the fact outside the branch that computes the state.

// Same reader, with the second fact reported alongside the state instead of
// being collapsed into it.
import { cmpSemver } from './status.mjs';

export function readStatus({ name, lastTag, versionOnMain, versionOnDev }) {
  let state;
  if (!lastTag) state = 'untagged-bump-on-main';
  else if (cmpSemver(versionOnMain, lastTag) > 0) state = 'untagged-bump-on-main';
  else if (cmpSemver(versionOnDev, versionOnMain) > 0) state = 'bump-on-dev-unpromoted';
  else state = 'clean';

  // A FACT, not a state. Computed outside the branch above so it is also set
  // on a component's never-released first bump, which reaches the same state
  // by a different route. Folding this into `state` is the bug: "main has an
  // untagged bump" and "dev already carries something higher" are
  // independently true, and one mutually-exclusive string reports only one.
  const devAhead =
    versionOnMain && versionOnDev && cmpSemver(versionOnDev, versionOnMain) > 0
      ? { version: versionOnDev, aheadOfMain: true }
      : null;

  // Scoped to exactly the state where the fast path is armed. `bump-on-dev-
  // unpromoted` also has devAhead set; that is its normal shape, and flagging
  // it would permanently mark a routine state as blocked, which is how a
  // blocker stops being read.
  const blockers = [];
  if (devAhead && state === 'untagged-bump-on-main') {
    blockers.push({
      id: 'dev-ahead-of-main',
      detail:
        `main carries ${versionOnMain} but dev carries ${devAhead.version}: ` +
        `cutting here would tag v${versionOnMain}, not v${devAhead.version}.`,
    });
  }

  return { name, state, lastTag, versionOnMain, versionOnDev, devAhead, blockers };
}

Two decisions in there are load-bearing, and both are easy to get wrong in the opposite direction.

devAhead is computed unconditionally, not inside the branch it usually accompanies. A component that has never been released reaches untagged-bump-on-main down a different path, and a patch written only against the branch you were looking at when you found the bug misses that sibling completely.

The blocker, by contrast, is scoped tightly. bump-on-dev-unpromoted also has devAhead set, and that is simply what that state looks like when everything is fine. Raising a blocker there would put a permanent warning on a routine condition, and a warning that is always on is a warning nobody reads.

Decide the target once, in a pure function

The second half of the bug was that the target version was derived twice, about ten lines apart, once preferring dev and once preferring main. Two derivations of the same decision are two chances to disagree, and this pair did.

// The ONE place the release target is decided. Pure: a status plus an optional
// operator-supplied version in, a decision out. No I/O, so it is trivially
// testable and can be called before anything irreversible happens.
//
// `requestedVersion` is a CONFIRMATION, not a bypass: it is only ever accepted
// when it names a version already present in this status, so there is no value
// of it that releases something which is not actually on the branch.
export function resolveReleaseTarget(status, requestedVersion = null) {
  const { name, state, versionOnMain, versionOnDev, devAhead } = status;

  if (state === 'untagged-bump-on-main') {
    if (!devAhead) {
      return { ok: true, version: versionOnMain, via: 'dispatch-on-main' };
    }
    if (requestedVersion === versionOnMain) {
      return {
        ok: true, version: versionOnMain, via: 'dispatch-on-main', confirmed: true,
      };
    }
    if (requestedVersion === versionOnDev) {
      return {
        ok: false,
        error: `${versionOnDev} is on dev but not on main, and a dispatch on main ` +
          `cannot cut it. Promote dev to main first, then re-read status.`,
      };
    }
    return {
      ok: false,
      error: `${name}: main carries ${versionOnMain} but dev carries ${versionOnDev}, ` +
        `so which one to release is ambiguous and this refuses to guess. Promote ` +
        `dev to main and re-read status to release ${versionOnDev}, or pass ` +
        `--version ${versionOnMain} to release exactly what is on main.`,
    };
  }

  // Every other state has a single unambiguous candidate: dev when it carries
  // the prepared bump, otherwise main.
  return { ok: true, version: versionOnDev ?? versionOnMain, via: 'prepared-branch' };
}

Purity is what makes this cheap to trust. There is no clone, no API call and no clock in it, so every branch can be exercised in a unit test in microseconds, and it can be called at the top of the dangerous function before anything has happened yet.

Then the caller uses its answer for everything, including the tag it later waits for:

// The caller. One resolve, before any network call, and both the dispatch and
// the tag it waits for read the same decided version.
import { resolveReleaseTarget } from './target.mjs';

// Stand-ins for the irreversible parts. In a real tool these dispatch a
// workflow and poll a remote; here they just record that they were reached.
const dispatched = [];
export function dispatchRelease(ref, version) {
  dispatched.push({ ref, version });
  return { ok: true };
}
export function reset() { dispatched.length = 0; }
export function calls() { return [...dispatched]; }

export function cut(status, { version = null } = {}) {
  const target = resolveReleaseTarget(status, version);
  if (!target.ok) return { ok: false, error: target.error };

  // Derived ONCE from the decided version. Before this existed the target was
  // computed twice, ten lines apart, and the two copies could disagree.
  const tag = `v${target.version}`;

  if (target.via === 'dispatch-on-main') {
    dispatchRelease('main', target.version);
    return { ok: true, tag, targetVersion: target.version, via: target.via };
  }
  dispatchRelease('release-branch', target.version);
  return { ok: true, tag, targetVersion: target.version, via: target.via };
}

That const tag line is the property worth naming. The thing you dispatch and the thing you then verify have to come from one value, or a run can succeed at proving the wrong artifact exists.

Make the escape hatch a confirmation, not a bypass

A refusal that cannot be overridden becomes a refusal people work around, so --version exists. The design constraint is that it must never be able to release something that is not there.

That is why the flag is checked against the status rather than trusted: --version 0.2.1 is accepted only because 0.2.1 is genuinely on main. --version 0.3.0 is refused even though 0.3.0 is a real version on a real branch, because a dispatch on main cannot cut it. There is no value of the flag that produces a release of something absent from the branch being dispatched.

The Command Line Interface Guidelines reach the same place from the usability side. Their advice for severe actions is to make confirmation hard to do by accident, “such as the name of the thing they’re deleting”, and they suggest a scriptable form: “Let them alternatively pass a flag such as --confirm="name-of-thing"”. A version number is exactly that. Typing it means you looked.

Run the cases

import assert from 'node:assert/strict';
import { readStatus as readCollapsed } from './status.mjs';
import { readStatus } from './status2.mjs';
import { cut, calls, reset } from './cut.mjs';

// The case that bites: main has an untagged bump AND dev carries something
// higher. Both facts are true at once.
const AMBIGUOUS = { name: 'widget', lastTag: '0.1.0', versionOnMain: '0.2.1', versionOnDev: '0.3.0' };

const collapsed = readCollapsed(AMBIGUOUS);
console.log('collapsed :', JSON.stringify(collapsed));
assert.equal(collapsed.state, 'untagged-bump-on-main');
console.log('  -> one state string, and nothing in it mentions 0.3.0\n');

const status = readStatus(AMBIGUOUS);
console.log('with fact :', JSON.stringify({ state: status.state, devAhead: status.devAhead }));
console.log('  blocker :', status.blockers[0].id, '|', status.blockers[0].detail, '\n');

// 1. Unconfirmed: refuse, and name both candidates.
reset();
const refused = cut(status);
assert.equal(refused.ok, false);
assert.equal(calls().length, 0, 'nothing irreversible may run on an ambiguous target');
console.log('unconfirmed cut ->', refused.error, '\n');

// 2. Confirmed with the version that IS on main: allowed.
reset();
const confirmed = cut(status, { version: '0.2.1' });
console.log('cut --version 0.2.1 ->', JSON.stringify(confirmed));
assert.deepEqual(calls(), [{ ref: 'main', version: '0.2.1' }]);

// 3. Confirmed with dev's version: still refused, because a dispatch on main
//    cannot cut a version that is not on main. The flag is not a bypass.
reset();
const wrong = cut(status, { version: '0.3.0' });
assert.equal(wrong.ok, false);
assert.equal(calls().length, 0);
console.log('cut --version 0.3.0 ->', wrong.error);

// 4. A version on neither branch is refused too.
reset();
assert.equal(cut(status, { version: '9.9.9' }).ok, false);
assert.equal(calls().length, 0);

// 5. The routine case still works untouched: dev ahead, but main is tagged, so
//    no fast path is armed and no blocker is raised.
const routine = readStatus({ name: 'widget', lastTag: '0.2.1', versionOnMain: '0.2.1', versionOnDev: '0.3.0' });
assert.equal(routine.state, 'bump-on-dev-unpromoted');
assert.ok(routine.devAhead, 'the fact is still reported');
assert.equal(routine.blockers.length, 0, 'but a routine state is never flagged');
console.log('\nroutine dev-ahead:', routine.state, '| blockers:', routine.blockers.length);

// 6. The sibling: a component that has never been released reaches the same
//    state by a different route, and must still be caught.
const firstEver = readStatus({ name: 'widget', lastTag: null, versionOnMain: '0.1.0', versionOnDev: '0.2.0' });
assert.equal(firstEver.state, 'untagged-bump-on-main');
assert.equal(firstEver.blockers.length, 1, 'a first release is not exempt');
console.log('never released :', firstEver.state, '| blockers:', firstEver.blockers.length);

console.log('\nall checks passed');

Save the four blocks as status.mjs, status2.mjs, target.mjs and cut.mjs, this one as check.mjs, and run node check.mjs. No dependencies; it is Node’s own assert module. Here is what it printed for me:

collapsed : {"name":"widget","state":"untagged-bump-on-main","lastTag":"0.1.0","versionOnMain":"0.2.1","versionOnDev":"0.3.0"}
  -> one state string, and nothing in it mentions 0.3.0

with fact : {"state":"untagged-bump-on-main","devAhead":{"version":"0.3.0","aheadOfMain":true}}
  blocker : dev-ahead-of-main | main carries 0.2.1 but dev carries 0.3.0: cutting here would tag v0.2.1, not v0.3.0.

unconfirmed cut -> widget: main carries 0.2.1 but dev carries 0.3.0, so which one to release is ambiguous and this refuses to guess. Promote dev to main and re-read status to release 0.3.0, or pass --version 0.2.1 to release exactly what is on main.

cut --version 0.2.1 -> {"ok":true,"tag":"v0.2.1","targetVersion":"0.2.1","via":"dispatch-on-main"}
cut --version 0.3.0 -> 0.3.0 is on dev but not on main, and a dispatch on main cannot cut it. Promote dev to main first, then re-read status.

routine dev-ahead: bump-on-dev-unpromoted | blockers: 0
never released : untagged-bump-on-main | blockers: 1

all checks passed

The assertion doing the most work is calls().length === 0. It is not checking that an error was returned; it is checking that the irreversible thing was never reached. Those are different tests, and only the second one survives someone later adding an early dispatch above the guard.

Gotchas

Patching only the branch you were standing in leaves the sibling. The bug surfaced on a component with an existing tag, so the obvious fix is to add a condition to that branch of the if/else chain. That fix is wrong for a component with no tags at all, which reaches the same state through the !lastTag arm and is just as capable of being mis-tagged. The escape is to ask which routes reach the dangerous state, not which one you happened to reproduce, and to compute the guarding fact outside the chain entirely.

A blocker raised in every state where the fact is true stops being a blocker. devAhead is set in two states and dangerous in one. Flagging both would put a permanent warning on a normal condition, and people learn very quickly to scroll past a warning that is always there. Scope the alarm to the state where the armed path actually exists, and be willing to leave the fact unflagged elsewhere.

Two derivations of one decision will eventually disagree. The version being dispatched and the tag being waited for were computed separately, which is what allowed a run to prove the existence of a tag it had not actually asked for. Whenever a function does a thing and then verifies the thing, both halves must read one variable. If you cannot point at the single expression that decided it, you have two.

Do not compare versions as strings. cmpSemver parses to numbers on purpose. Semantic Versioning requires numeric comparison: “Major, minor, and patch versions are always compared numerically”, and the spec forbids leading zeroes for the same reason. Lexicographic order puts 0.10.0 below 0.9.0, and in a status reader that mistake reads as “dev is behind” and quietly disarms the check you just added.

Prove the refusal blocks the side effect, not just the return value. A test asserting ok === false passes perfectly well against a function that already dispatched before returning. Give the irreversible call a recorder, and assert it was never invoked.

Sources

Changelog

  • fix(shipflow): refuse the ambiguous release-cut fast path (0.6.0) (#174) (249d4fd)
  • fix: a lockfile that disagrees with its package.json is a lie about what shipped (#163) (b41c1a2)