fix(reset): the verdict compares bytes, a broken promise fails the run, and the rule comes back on the NIC

internal/cli/checkpoint.go

@@ -165,25 +165,16 @@ func resetCommand(args []string, stdout, stderr io.Writer) int {
 	}
 	endpoint := "http://" + *addr
 
-	recorded, err := readRecordedCheckpoint(*addr)
+	// The name chooses the record, and an empty one means the last taken.
+	// Before the audit this read one file per instance, so a second
+	// checkpoint made the first unresettable.
+	recorded, err := readRecordedCheckpoint(*addr, asked)
 	if err != nil {
 		fmt.Fprintf(stderr, "feint reset: %v\n", err)
 		fmt.Fprintf(stderr, "  Take one first: feint checkpoint <name> --addr %s\n", *addr)
 		return exitError
 	}
 	name := recorded.Name
-	if asked != "" {
-		name = asked
-	}
-	if name != recorded.Name {
-		// Refused rather than guessed. The recorded state is what a verdict
-		// compares against, and comparing one checkpoint's emulator state to
-		// another's observation would report differences that are not the
-		// reset's fault.
-		fmt.Fprintf(stderr, "feint reset: this instance recorded %q, not %q, so a verdict on %q would compare the wrong states\n",
-			recorded.Name, name, name)
-		return exitError
-	}
 
 	answer, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/reset/"+name)
 	if err != nil {
@@ -306,18 +297,45 @@ func callCheckpoint(method, url string) (map[string]any, error) {
 	return body, nil
 }
 
-// checkpointPath is where the observed state lives: beside the instance
-// record, because it belongs to that emulator and goes when it does.
-func checkpointPath(addr string) (string, error) {
+// checkpointPath is where one checkpoint's observed state lives: beside the
+// instance record, because it belongs to that emulator and goes when it does.
+//
+// Indexed by NAME, which the first version was not. An external audit caught
+// it: the emulator holds any number of named checkpoints and the CLI wrote a
+// single checkpoint.json, so `checkpoint A` followed by `checkpoint B` made
+// `reset A` impossible — the observation it would have been judged against had
+// been overwritten. The refusal that fired was honest and the feature was
+// still missing.
+//
+// The name has already been through checkpointArgName or is a session
+// identity this command built, so it carries no separator. It is cleaned here
+// anyway, because a path built from a name is a path whoever reads this file
+// next will assume was checked.
+//
+// TestTwoNamedCheckpointsDoNotOverwriteEachOther fails without this.
+func checkpointPath(addr, name string) (string, error) {
+	dir, err := instanceDir(addr)
+	if err != nil {
+		return "", err
+	}
+	if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) {
+		return "", fmt.Errorf("%q is not a checkpoint name", name)
+	}
+	return filepath.Join(dir, "checkpoints", filepath.Base(name)+".json"), nil
+}
+
+// lastCheckpointPath records which name was taken most recently, so that
+// `feint reset` with no argument has something to mean.
+func lastCheckpointPath(addr string) (string, error) {
 	dir, err := instanceDir(addr)
 	if err != nil {
 		return "", err
 	}
-	return filepath.Join(dir, "checkpoint.json"), nil
+	return filepath.Join(dir, "checkpoints", "last"), nil
 }
 
 func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error {
-	path, err := checkpointPath(addr)
+	path, err := checkpointPath(addr, recorded.Name)
 	if err != nil {
 		return err
 	}
@@ -328,17 +346,37 @@ func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error {
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(path, append(body, '\n'), 0o600)
+	if err := os.WriteFile(path, append(body, '\n'), 0o600); err != nil {
+		return err
+	}
+	// Written after the record, so a failure above leaves `last` pointing at
+	// a checkpoint that exists.
+	last, err := lastCheckpointPath(addr)
+	if err != nil {
+		return err
+	}
+	return os.WriteFile(last, []byte(recorded.Name+"\n"), 0o600)
 }
 
-func readRecordedCheckpoint(addr string) (recordedCheckpoint, error) {
-	path, err := checkpointPath(addr)
+func readRecordedCheckpoint(addr, name string) (recordedCheckpoint, error) {
+	if name == "" {
+		last, err := lastCheckpointPath(addr)
+		if err != nil {
+			return recordedCheckpoint{}, err
+		}
+		body, err := os.ReadFile(filepath.Clean(last))
+		if err != nil {
+			return recordedCheckpoint{}, fmt.Errorf("no checkpoint recorded for %s", addr)
+		}
+		name = strings.TrimSpace(string(body))
+	}
+	path, err := checkpointPath(addr, name)
 	if err != nil {
 		return recordedCheckpoint{}, err
 	}
 	body, err := os.ReadFile(filepath.Clean(path))
 	if err != nil {
-		return recordedCheckpoint{}, fmt.Errorf("no checkpoint recorded for %s", addr)
+		return recordedCheckpoint{}, fmt.Errorf("no checkpoint named %q recorded for %s", name, addr)
 	}
 	var recorded recordedCheckpoint
 	if err := json.Unmarshal(body, &recorded); err != nil {
@@ -386,7 +424,7 @@ func takeSessionCheckpoint(endpoint, session, addr string) error {
 //
 // TestAFailedResetAfterIsReportedAndNotRaised fails without this.
 func resetToSession(endpoint, session, addr string, stderr io.Writer) (string, []string) {
-	recorded, err := readRecordedCheckpoint(addr)
+	recorded, err := readRecordedCheckpoint(addr, session)
 	if err != nil || recorded.Name != session {
 		fmt.Fprintf(stderr, "feint run: --reset-after has no session checkpoint to return to: %v\n", err)
 		return "not attempted", nil

internal/cli/checkpoint_test.go

@@ -55,15 +55,16 @@ func TestACheckpointNameIsTakenBeforeTheFlags(t *testing.T) {
 	}
 }
 
-// TestAResetRefusesToJudgeAgainstAnotherCheckpointsState is the other half of
-// the verdict's honesty.
+// TestAResetNamesTheCheckpointItCouldNotFind replaces a refusal that lost its
+// subject to the audit's second finding.
 //
-// The recorded observable state belongs to ONE checkpoint. Comparing the
-// emulator's restore of one against the observation of another would report
-// differences that are not the reset's fault, and an operator would chase
-// them. So a name that disagrees with the record is refused rather than
-// guessed.
-func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) {
+// Before, the CLI kept one observation per instance, so asking to reset a
+// name it had not recorded last meant comparing one checkpoint's restore to
+// another's observation — and the command refused, honestly, while the
+// feature was missing. Observations are indexed by name now, so an unknown
+// name is simply unknown, and what matters is that the refusal says WHICH
+// name and how to make one.
+func TestAResetNamesTheCheckpointItCouldNotFind(t *testing.T) {
 	t.Setenv("XDG_RUNTIME_DIR", t.TempDir())
 	const addr = "127.0.0.1:4599"
 	if err := writeRecordedCheckpoint(addr, recordedCheckpoint{
@@ -74,9 +75,9 @@ func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) {
 
 	var stdout, stderr bytes.Buffer
 	if code := resetCommand([]string{"something-else", "--addr", addr}, &stdout, &stderr); code != exitError {
-		t.Fatalf("a reset against another checkpoint's state was allowed (%d)", code)
+		t.Fatalf("a reset of a name nobody recorded was allowed (%d)", code)
 	}
-	for _, want := range []string{"baseline", "something-else", "compare the wrong states"} {
+	for _, want := range []string{"something-else", "feint checkpoint"} {
 		if !strings.Contains(stderr.String(), want) {
 			t.Errorf("the refusal does not mention %q: %s", want, stderr.String())
 		}
@@ -94,6 +95,51 @@ func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) {
 	}
 }
 
+// TestTwoNamedCheckpointsDoNotOverwriteEachOther is cited by checkpointPath,
+// and it is the audit's second finding.
+//
+// The emulator holds any number of named checkpoints; the CLI wrote a single
+// file per instance. So `checkpoint A` then `checkpoint B` made `reset A`
+// impossible, because the observation A would have been judged against had
+// been overwritten by B's.
+func TestTwoNamedCheckpointsDoNotOverwriteEachOther(t *testing.T) {
+	t.Setenv("XDG_RUNTIME_DIR", t.TempDir())
+	const addr = "127.0.0.1:4599"
+
+	for _, name := range []string{"alpha", "beta"} {
+		if err := writeRecordedCheckpoint(addr, recordedCheckpoint{
+			SchemaVersion: checkpointStateVersion, Name: name,
+			State: observed{Machines: []string{"feint-scw-" + name}},
+		}); err != nil {
+			t.Fatalf("record %s: %v", name, err)
+		}
+	}
+
+	// Both are still readable BY NAME, which is what the single file broke.
+	for _, name := range []string{"alpha", "beta"} {
+		back, err := readRecordedCheckpoint(addr, name)
+		if err != nil {
+			t.Fatalf("read %s back: %v", name, err)
+		}
+		if back.Name != name {
+			t.Errorf("reading %q answered %q", name, back.Name)
+		}
+		if len(back.State.Machines) != 1 || back.State.Machines[0] != "feint-scw-"+name {
+			t.Errorf("%s carries the wrong observation: %v", name, back.State.Machines)
+		}
+	}
+
+	// And an empty name means the last taken, so `feint reset` with no
+	// argument has something to mean.
+	last, err := readRecordedCheckpoint(addr, "")
+	if err != nil {
+		t.Fatalf("read the last: %v", err)
+	}
+	if last.Name != "beta" {
+		t.Errorf("the last taken reads as %q", last.Name)
+	}
+}
+
 // TestARecordedCheckpointRefusesAVersionItCannotRead is the forward
 // compatibility half.
 //
@@ -109,7 +155,7 @@ func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) {
 	}); err != nil {
 		t.Fatalf("record a checkpoint: %v", err)
 	}
-	if _, err := readRecordedCheckpoint(addr); err == nil {
+	if _, err := readRecordedCheckpoint(addr, ""); err == nil {
 		t.Fatal("a record from a later version was read anyway")
 	} else if !strings.Contains(err.Error(), "version") {
 		t.Errorf("the refusal does not say it is a version: %v", err)
@@ -123,7 +169,7 @@ func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) {
 	}); err != nil {
 		t.Fatalf("record a checkpoint: %v", err)
 	}
-	back, err := readRecordedCheckpoint(addr)
+	back, err := readRecordedCheckpoint(addr, "")
 	if err != nil {
 		t.Fatalf("read it back: %v", err)
 	}

internal/cli/exit_contract_test.go

@@ -0,0 +1,64 @@
+package cli
+
+import (
+	"testing"
+)
+
+// TestAGuaranteeThatWasNotDeliveredFailsTheRun is cited by runCommand's exit
+// code, and it closes a contract error an external audit found in merged code.
+//
+// The first version failed the run only on "did not converge". So this was a
+// possible CI run:
+//
+//	terraform test   success
+//	feint reset      failed
+//	feint run        exit 0
+//
+// and the same held for a lab this command owned and could not stop: the
+// receipt said `cleanup: failed` and the exit code stayed at the child's.
+//
+// The table is written as the full cross product on purpose. Two of the
+// verdict words — "unverified" and "not attempted" — are the ones the first
+// version let through, and a test that only covered the word it already
+// handled would have agreed with the defect.
+func TestAGuaranteeThatWasNotDeliveredFailsTheRun(t *testing.T) {
+	for _, probe := range []struct {
+		name      string
+		reset     string
+		cleanup   string
+		delivered bool
+	}{
+		// No reset was asked for, so there is nothing to have failed.
+		{"nothing asked, clean teardown", "", "clean", true},
+		{"nothing asked, lab not ours", "", "not owned", true},
+		{"nothing asked, kept on purpose", "", "kept", true},
+
+		// A reset that was asked for and delivered.
+		{"reset converged", "converged", "clean", true},
+
+		// Every way a reset can fail to deliver. The first version of this
+		// guard treated the last three as success.
+		{"reset did not converge", "did not converge", "clean", false},
+		{"the emulator refused the reset", "failed", "clean", false},
+		{"the reset could not be read back", "unverified", "clean", false},
+		{"there was no checkpoint to return to", "not attempted", "clean", false},
+
+		// A teardown this command owed and did not perform. The receipt said
+		// so and the exit code did not.
+		{"the lab could not be stopped", "", "failed", false},
+		{"converged, and the lab leaked anyway", "converged", "failed", false},
+
+		// A word nobody has written yet must not default to success: that is
+		// the whole reason the function lists the failures rather than the
+		// successes.
+		{"an unknown verdict word", "something-new", "clean", false},
+		{"an unknown cleanup word", "", "something-new", false},
+	} {
+		t.Run(probe.name, func(t *testing.T) {
+			if got := deliveredWhatWasAsked(probe.reset, probe.cleanup); got != probe.delivered {
+				t.Errorf("deliveredWhatWasAsked(%q, %q) = %v, want %v",
+					probe.reset, probe.cleanup, got, probe.delivered)
+			}
+		})
+	}
+}